[PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
are thus fundamental to PCIe being usable at all. As such, it makes
sense to treat them as non-optional and rely on dummy regulators if
not explicitly described.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
 1 file changed, 25 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index ef8e677ce9d1..68525f8ac4d9 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		dev_info(dev, "no vpcie3v3 regulator found\n");
 	}
 
-	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
-	if (IS_ERR(rockchip->vpcie1v8)) {
-		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
-			return PTR_ERR(rockchip->vpcie1v8);
-		dev_info(dev, "no vpcie1v8 regulator found\n");
-	}
+	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
+	if (IS_ERR(rockchip->vpcie1v8))
+		return PTR_ERR(rockchip->vpcie1v8);
 
-	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
-	if (IS_ERR(rockchip->vpcie0v9)) {
-		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
-			return PTR_ERR(rockchip->vpcie0v9);
-		dev_info(dev, "no vpcie0v9 regulator found\n");
-	}
+	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
+	if (IS_ERR(rockchip->vpcie0v9))
+		return PTR_ERR(rockchip->vpcie0v9);
 
 	return 0;
 }
@@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 		}
 	}
 
-	if (!IS_ERR(rockchip->vpcie1v8)) {
-		err = regulator_enable(rockchip->vpcie1v8);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
-			goto err_disable_3v3;
-		}
+	err = regulator_enable(rockchip->vpcie1v8);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
+		goto err_disable_3v3;
 	}
 
-	if (!IS_ERR(rockchip->vpcie0v9)) {
-		err = regulator_enable(rockchip->vpcie0v9);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-			goto err_disable_1v8;
-		}
+	err = regulator_enable(rockchip->vpcie0v9);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
+		goto err_disable_1v8;
 	}
 
 	return 0;
 
 err_disable_1v8:
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie1v8);
 err_disable_3v3:
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
@@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
 
 	rockchip_pcie_disable_clocks(rockchip);
 
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie0v9);
 
 	return ret;
 }
@@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
 	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
 	int err;
 
-	if (!IS_ERR(rockchip->vpcie0v9)) {
-		err = regulator_enable(rockchip->vpcie0v9);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-			return err;
-		}
+	err = regulator_enable(rockchip->vpcie0v9);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
+		return err;
 	}
 
 	err = rockchip_pcie_enable_clocks(rockchip);
@@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
 err_pcie_resume:
 	rockchip_pcie_disable_clocks(rockchip);
 err_disable_0v9:
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie0v9);
 	return err;
 }
 
@@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 		regulator_disable(rockchip->vpcie12v);
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie0v9);
 err_set_vpcie:
 	rockchip_pcie_disable_clocks(rockchip);
 	return err;
@@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
 		regulator_disable(rockchip->vpcie12v);
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie0v9);
 
 	return 0;
 }
-- 
2.17.1

[PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
are thus fundamental to PCIe being usable at all. As such, it makes
sense to treat them as non-optional and rely on dummy regulators if
not explicitly described.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
 1 file changed, 25 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index ef8e677ce9d1..68525f8ac4d9 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		dev_info(dev, "no vpcie3v3 regulator found\n");
 	}
 
-	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
-	if (IS_ERR(rockchip->vpcie1v8)) {
-		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
-			return PTR_ERR(rockchip->vpcie1v8);
-		dev_info(dev, "no vpcie1v8 regulator found\n");
-	}
+	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
+	if (IS_ERR(rockchip->vpcie1v8))
+		return PTR_ERR(rockchip->vpcie1v8);
 
-	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
-	if (IS_ERR(rockchip->vpcie0v9)) {
-		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
-			return PTR_ERR(rockchip->vpcie0v9);
-		dev_info(dev, "no vpcie0v9 regulator found\n");
-	}
+	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
+	if (IS_ERR(rockchip->vpcie0v9))
+		return PTR_ERR(rockchip->vpcie0v9);
 
 	return 0;
 }
@@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 		}
 	}
 
-	if (!IS_ERR(rockchip->vpcie1v8)) {
-		err = regulator_enable(rockchip->vpcie1v8);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
-			goto err_disable_3v3;
-		}
+	err = regulator_enable(rockchip->vpcie1v8);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
+		goto err_disable_3v3;
 	}
 
-	if (!IS_ERR(rockchip->vpcie0v9)) {
-		err = regulator_enable(rockchip->vpcie0v9);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-			goto err_disable_1v8;
-		}
+	err = regulator_enable(rockchip->vpcie0v9);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
+		goto err_disable_1v8;
 	}
 
 	return 0;
 
 err_disable_1v8:
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie1v8);
 err_disable_3v3:
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
@@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
 
 	rockchip_pcie_disable_clocks(rockchip);
 
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie0v9);
 
 	return ret;
 }
@@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
 	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
 	int err;
 
-	if (!IS_ERR(rockchip->vpcie0v9)) {
-		err = regulator_enable(rockchip->vpcie0v9);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-			return err;
-		}
+	err = regulator_enable(rockchip->vpcie0v9);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
+		return err;
 	}
 
 	err = rockchip_pcie_enable_clocks(rockchip);
@@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
 err_pcie_resume:
 	rockchip_pcie_disable_clocks(rockchip);
 err_disable_0v9:
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie0v9);
 	return err;
 }
 
@@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 		regulator_disable(rockchip->vpcie12v);
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie0v9);
 err_set_vpcie:
 	rockchip_pcie_disable_clocks(rockchip);
 	return err;
@@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
 		regulator_disable(rockchip->vpcie12v);
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie0v9);
 
 	return 0;
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
are thus fundamental to PCIe being usable at all. As such, it makes
sense to treat them as non-optional and rely on dummy regulators if
not explicitly described.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
 1 file changed, 25 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index ef8e677ce9d1..68525f8ac4d9 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		dev_info(dev, "no vpcie3v3 regulator found\n");
 	}
 
-	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
-	if (IS_ERR(rockchip->vpcie1v8)) {
-		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
-			return PTR_ERR(rockchip->vpcie1v8);
-		dev_info(dev, "no vpcie1v8 regulator found\n");
-	}
+	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
+	if (IS_ERR(rockchip->vpcie1v8))
+		return PTR_ERR(rockchip->vpcie1v8);
 
-	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
-	if (IS_ERR(rockchip->vpcie0v9)) {
-		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
-			return PTR_ERR(rockchip->vpcie0v9);
-		dev_info(dev, "no vpcie0v9 regulator found\n");
-	}
+	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
+	if (IS_ERR(rockchip->vpcie0v9))
+		return PTR_ERR(rockchip->vpcie0v9);
 
 	return 0;
 }
@@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 		}
 	}
 
-	if (!IS_ERR(rockchip->vpcie1v8)) {
-		err = regulator_enable(rockchip->vpcie1v8);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
-			goto err_disable_3v3;
-		}
+	err = regulator_enable(rockchip->vpcie1v8);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
+		goto err_disable_3v3;
 	}
 
-	if (!IS_ERR(rockchip->vpcie0v9)) {
-		err = regulator_enable(rockchip->vpcie0v9);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-			goto err_disable_1v8;
-		}
+	err = regulator_enable(rockchip->vpcie0v9);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
+		goto err_disable_1v8;
 	}
 
 	return 0;
 
 err_disable_1v8:
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie1v8);
 err_disable_3v3:
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
@@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
 
 	rockchip_pcie_disable_clocks(rockchip);
 
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie0v9);
 
 	return ret;
 }
@@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
 	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
 	int err;
 
-	if (!IS_ERR(rockchip->vpcie0v9)) {
-		err = regulator_enable(rockchip->vpcie0v9);
-		if (err) {
-			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-			return err;
-		}
+	err = regulator_enable(rockchip->vpcie0v9);
+	if (err) {
+		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
+		return err;
 	}
 
 	err = rockchip_pcie_enable_clocks(rockchip);
@@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
 err_pcie_resume:
 	rockchip_pcie_disable_clocks(rockchip);
 err_disable_0v9:
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie0v9);
 	return err;
 }
 
@@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 		regulator_disable(rockchip->vpcie12v);
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie0v9);
 err_set_vpcie:
 	rockchip_pcie_disable_clocks(rockchip);
 	return err;
@@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
 		regulator_disable(rockchip->vpcie12v);
 	if (!IS_ERR(rockchip->vpcie3v3))
 		regulator_disable(rockchip->vpcie3v3);
-	if (!IS_ERR(rockchip->vpcie1v8))
-		regulator_disable(rockchip->vpcie1v8);
-	if (!IS_ERR(rockchip->vpcie0v9))
-		regulator_disable(rockchip->vpcie0v9);
+	regulator_disable(rockchip->vpcie1v8);
+	regulator_disable(rockchip->vpcie0v9);
 
 	return 0;
 }
-- 
2.17.1

[PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

Null checks are both cheaper and more readable than having !IS_ERR()
splattered everywhere.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 68525f8ac4d9..a61819efc0c1 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -262,7 +262,7 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 	int curr;
 	u32 status, scale, power;
 
-	if (IS_ERR(rockchip->vpcie3v3))
+	if (!rockchip->vpcie3v3)
 		return;
 
 	/*
@@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
 			return PTR_ERR(rockchip->vpcie12v);
 		dev_info(dev, "no vpcie12v regulator found\n");
+		rockchip->vpcie12v = NULL;
 	}
 
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
@@ -618,6 +619,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
 			return PTR_ERR(rockchip->vpcie3v3);
 		dev_info(dev, "no vpcie3v3 regulator found\n");
+		rockchip->vpcie3v3 = NULL;
 	}
 
 	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
@@ -636,7 +638,7 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 	struct device *dev = rockchip->dev;
 	int err;
 
-	if (!IS_ERR(rockchip->vpcie12v)) {
+	if (rockchip->vpcie12v) {
 		err = regulator_enable(rockchip->vpcie12v);
 		if (err) {
 			dev_err(dev, "fail to enable vpcie12v regulator\n");
@@ -644,7 +646,7 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 		}
 	}
 
-	if (!IS_ERR(rockchip->vpcie3v3)) {
+	if (rockchip->vpcie3v3) {
 		err = regulator_enable(rockchip->vpcie3v3);
 		if (err) {
 			dev_err(dev, "fail to enable vpcie3v3 regulator\n");
@@ -669,10 +671,10 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 err_disable_1v8:
 	regulator_disable(rockchip->vpcie1v8);
 err_disable_3v3:
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 err_disable_12v:
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
 err_out:
 	return err;
@@ -1062,9 +1064,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 err_deinit_port:
 	rockchip_pcie_deinit_phys(rockchip);
 err_vpcie:
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 	regulator_disable(rockchip->vpcie1v8);
 	regulator_disable(rockchip->vpcie0v9);
@@ -1087,9 +1089,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
 
 	rockchip_pcie_disable_clocks(rockchip);
 
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 	regulator_disable(rockchip->vpcie1v8);
 	regulator_disable(rockchip->vpcie0v9);
-- 
2.17.1

[PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

Null checks are both cheaper and more readable than having !IS_ERR()
splattered everywhere.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 68525f8ac4d9..a61819efc0c1 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -262,7 +262,7 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 	int curr;
 	u32 status, scale, power;
 
-	if (IS_ERR(rockchip->vpcie3v3))
+	if (!rockchip->vpcie3v3)
 		return;
 
 	/*
@@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
 			return PTR_ERR(rockchip->vpcie12v);
 		dev_info(dev, "no vpcie12v regulator found\n");
+		rockchip->vpcie12v = NULL;
 	}
 
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
@@ -618,6 +619,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
 			return PTR_ERR(rockchip->vpcie3v3);
 		dev_info(dev, "no vpcie3v3 regulator found\n");
+		rockchip->vpcie3v3 = NULL;
 	}
 
 	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
@@ -636,7 +638,7 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 	struct device *dev = rockchip->dev;
 	int err;
 
-	if (!IS_ERR(rockchip->vpcie12v)) {
+	if (rockchip->vpcie12v) {
 		err = regulator_enable(rockchip->vpcie12v);
 		if (err) {
 			dev_err(dev, "fail to enable vpcie12v regulator\n");
@@ -644,7 +646,7 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 		}
 	}
 
-	if (!IS_ERR(rockchip->vpcie3v3)) {
+	if (rockchip->vpcie3v3) {
 		err = regulator_enable(rockchip->vpcie3v3);
 		if (err) {
 			dev_err(dev, "fail to enable vpcie3v3 regulator\n");
@@ -669,10 +671,10 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 err_disable_1v8:
 	regulator_disable(rockchip->vpcie1v8);
 err_disable_3v3:
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 err_disable_12v:
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
 err_out:
 	return err;
@@ -1062,9 +1064,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 err_deinit_port:
 	rockchip_pcie_deinit_phys(rockchip);
 err_vpcie:
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 	regulator_disable(rockchip->vpcie1v8);
 	regulator_disable(rockchip->vpcie0v9);
@@ -1087,9 +1089,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
 
 	rockchip_pcie_disable_clocks(rockchip);
 
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 	regulator_disable(rockchip->vpcie1v8);
 	regulator_disable(rockchip->vpcie0v9);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

Null checks are both cheaper and more readable than having !IS_ERR()
splattered everywhere.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 68525f8ac4d9..a61819efc0c1 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -262,7 +262,7 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 	int curr;
 	u32 status, scale, power;
 
-	if (IS_ERR(rockchip->vpcie3v3))
+	if (!rockchip->vpcie3v3)
 		return;
 
 	/*
@@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
 			return PTR_ERR(rockchip->vpcie12v);
 		dev_info(dev, "no vpcie12v regulator found\n");
+		rockchip->vpcie12v = NULL;
 	}
 
 	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
@@ -618,6 +619,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 		if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV)
 			return PTR_ERR(rockchip->vpcie3v3);
 		dev_info(dev, "no vpcie3v3 regulator found\n");
+		rockchip->vpcie3v3 = NULL;
 	}
 
 	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
@@ -636,7 +638,7 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 	struct device *dev = rockchip->dev;
 	int err;
 
-	if (!IS_ERR(rockchip->vpcie12v)) {
+	if (rockchip->vpcie12v) {
 		err = regulator_enable(rockchip->vpcie12v);
 		if (err) {
 			dev_err(dev, "fail to enable vpcie12v regulator\n");
@@ -644,7 +646,7 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 		}
 	}
 
-	if (!IS_ERR(rockchip->vpcie3v3)) {
+	if (rockchip->vpcie3v3) {
 		err = regulator_enable(rockchip->vpcie3v3);
 		if (err) {
 			dev_err(dev, "fail to enable vpcie3v3 regulator\n");
@@ -669,10 +671,10 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 err_disable_1v8:
 	regulator_disable(rockchip->vpcie1v8);
 err_disable_3v3:
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 err_disable_12v:
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
 err_out:
 	return err;
@@ -1062,9 +1064,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 err_deinit_port:
 	rockchip_pcie_deinit_phys(rockchip);
 err_vpcie:
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 	regulator_disable(rockchip->vpcie1v8);
 	regulator_disable(rockchip->vpcie0v9);
@@ -1087,9 +1089,9 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
 
 	rockchip_pcie_disable_clocks(rockchip);
 
-	if (!IS_ERR(rockchip->vpcie12v))
+	if (rockchip->vpcie12v)
 		regulator_disable(rockchip->vpcie12v);
-	if (!IS_ERR(rockchip->vpcie3v3))
+	if (rockchip->vpcie3v3)
 		regulator_disable(rockchip->vpcie3v3);
 	regulator_disable(rockchip->vpcie1v8);
 	regulator_disable(rockchip->vpcie0v9);
-- 
2.17.1

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Mark Brown]

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

On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> Null checks are both cheaper and more readable than having !IS_ERR()
> splattered everywhere.

> -	if (IS_ERR(rockchip->vpcie3v3))
> +	if (!rockchip->vpcie3v3)
>  		return;
>  
>  	/*
> @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
>  			return PTR_ERR(rockchip->vpcie12v);
>  		dev_info(dev, "no vpcie12v regulator found\n");
> +		rockchip->vpcie12v = NULL;

According to the API NULL is a valid regulator.  We don't currently
actually do this but it's storing up surprises if you treat it as
invalid.

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

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 679 bytes --]

On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> Null checks are both cheaper and more readable than having !IS_ERR()
> splattered everywhere.

> -	if (IS_ERR(rockchip->vpcie3v3))
> +	if (!rockchip->vpcie3v3)
>  		return;
>  
>  	/*
> @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
>  			return PTR_ERR(rockchip->vpcie12v);
>  		dev_info(dev, "no vpcie12v regulator found\n");
> +		rockchip->vpcie12v = NULL;

According to the API NULL is a valid regulator.  We don't currently
actually do this but it's storing up surprises if you treat it as
invalid.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

On 18/11/2019 11:59 am, Mark Brown wrote:
> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
>> Null checks are both cheaper and more readable than having !IS_ERR()
>> splattered everywhere.
> 
>> -	if (IS_ERR(rockchip->vpcie3v3))
>> +	if (!rockchip->vpcie3v3)
>>   		return;
>>   
>>   	/*
>> @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>   		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
>>   			return PTR_ERR(rockchip->vpcie12v);
>>   		dev_info(dev, "no vpcie12v regulator found\n");
>> +		rockchip->vpcie12v = NULL;
> 
> According to the API NULL is a valid regulator.  We don't currently
> actually do this but it's storing up surprises if you treat it as
> invalid.

Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable() 
immediately dereferencing its argument without any checks. If we'd 
rather not bake in that assumption then this patch can happily be ignored.

Thanks,
Robin.

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

On 18/11/2019 11:59 am, Mark Brown wrote:
> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
>> Null checks are both cheaper and more readable than having !IS_ERR()
>> splattered everywhere.
> 
>> -	if (IS_ERR(rockchip->vpcie3v3))
>> +	if (!rockchip->vpcie3v3)
>>   		return;
>>   
>>   	/*
>> @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>   		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
>>   			return PTR_ERR(rockchip->vpcie12v);
>>   		dev_info(dev, "no vpcie12v regulator found\n");
>> +		rockchip->vpcie12v = NULL;
> 
> According to the API NULL is a valid regulator.  We don't currently
> actually do this but it's storing up surprises if you treat it as
> invalid.

Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable() 
immediately dereferencing its argument without any checks. If we'd 
rather not bake in that assumption then this patch can happily be ignored.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

On 18/11/2019 11:59 am, Mark Brown wrote:
> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
>> Null checks are both cheaper and more readable than having !IS_ERR()
>> splattered everywhere.
> 
>> -	if (IS_ERR(rockchip->vpcie3v3))
>> +	if (!rockchip->vpcie3v3)
>>   		return;
>>   
>>   	/*
>> @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>   		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
>>   			return PTR_ERR(rockchip->vpcie12v);
>>   		dev_info(dev, "no vpcie12v regulator found\n");
>> +		rockchip->vpcie12v = NULL;
> 
> According to the API NULL is a valid regulator.  We don't currently
> actually do this but it's storing up surprises if you treat it as
> invalid.

Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable() 
immediately dereferencing its argument without any checks. If we'd 
rather not bake in that assumption then this patch can happily be ignored.

Thanks,
Robin.

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Andrew Murray]

On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
> On 18/11/2019 11:59 am, Mark Brown wrote:
> > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > splattered everywhere.
> > 
> > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > +	if (!rockchip->vpcie3v3)
> > >   		return;
> > >   	/*
> > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > >   		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > >   			return PTR_ERR(rockchip->vpcie12v);
> > >   		dev_info(dev, "no vpcie12v regulator found\n");
> > > +		rockchip->vpcie12v = NULL;
> > 
> > According to the API NULL is a valid regulator.  We don't currently
> > actually do this but it's storing up surprises if you treat it as
> > invalid.
> 
> Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
> immediately dereferencing its argument without any checks. If we'd rather
> not bake in that assumption then this patch can happily be ignored.

I'd suggest we drop this patch.

"IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
-4095 inclusive.

Regardless the effect of this patch with the regulator framework, I don't
think we want to create an example that others may follow.

Thanks,

Andrew Murray

> 
> Thanks,
> Robin.

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Andrew Murray]

On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
> On 18/11/2019 11:59 am, Mark Brown wrote:
> > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > splattered everywhere.
> > 
> > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > +	if (!rockchip->vpcie3v3)
> > >   		return;
> > >   	/*
> > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > >   		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > >   			return PTR_ERR(rockchip->vpcie12v);
> > >   		dev_info(dev, "no vpcie12v regulator found\n");
> > > +		rockchip->vpcie12v = NULL;
> > 
> > According to the API NULL is a valid regulator.  We don't currently
> > actually do this but it's storing up surprises if you treat it as
> > invalid.
> 
> Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
> immediately dereferencing its argument without any checks. If we'd rather
> not bake in that assumption then this patch can happily be ignored.

I'd suggest we drop this patch.

"IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
-4095 inclusive.

Regardless the effect of this patch with the regulator framework, I don't
think we want to create an example that others may follow.

Thanks,

Andrew Murray

> 
> Thanks,
> Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Andrew Murray]

On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
> On 18/11/2019 11:59 am, Mark Brown wrote:
> > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > splattered everywhere.
> > 
> > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > +	if (!rockchip->vpcie3v3)
> > >   		return;
> > >   	/*
> > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > >   		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > >   			return PTR_ERR(rockchip->vpcie12v);
> > >   		dev_info(dev, "no vpcie12v regulator found\n");
> > > +		rockchip->vpcie12v = NULL;
> > 
> > According to the API NULL is a valid regulator.  We don't currently
> > actually do this but it's storing up surprises if you treat it as
> > invalid.
> 
> Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
> immediately dereferencing its argument without any checks. If we'd rather
> not bake in that assumption then this patch can happily be ignored.

I'd suggest we drop this patch.

"IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
-4095 inclusive.

Regardless the effect of this patch with the regulator framework, I don't
think we want to create an example that others may follow.

Thanks,

Andrew Murray

> 
> Thanks,
> Robin.

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

On 18/11/2019 12:39 pm, Andrew Murray wrote:
> On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
>> On 18/11/2019 11:59 am, Mark Brown wrote:
>>> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
>>>> Null checks are both cheaper and more readable than having !IS_ERR()
>>>> splattered everywhere.
>>>
>>>> -	if (IS_ERR(rockchip->vpcie3v3))
>>>> +	if (!rockchip->vpcie3v3)
>>>>    		return;
>>>>    	/*
>>>> @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>>    		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
>>>>    			return PTR_ERR(rockchip->vpcie12v);
>>>>    		dev_info(dev, "no vpcie12v regulator found\n");
>>>> +		rockchip->vpcie12v = NULL;
>>>
>>> According to the API NULL is a valid regulator.  We don't currently
>>> actually do this but it's storing up surprises if you treat it as
>>> invalid.
>>
>> Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
>> immediately dereferencing its argument without any checks. If we'd rather
>> not bake in that assumption then this patch can happily be ignored.
> 
> I'd suggest we drop this patch.
> 
> "IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
> -4095 inclusive.

Hence the explicit initial "if (IS_ERR(ptr)) ptr = NULL;" condition 
quoted above ;)

But yeah, it was merely an attempt at a minor cosmetic cleanup, so let's 
just forget about it to avoid any possible confusion.

Cheers,
Robin.

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

On 18/11/2019 12:39 pm, Andrew Murray wrote:
> On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
>> On 18/11/2019 11:59 am, Mark Brown wrote:
>>> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
>>>> Null checks are both cheaper and more readable than having !IS_ERR()
>>>> splattered everywhere.
>>>
>>>> -	if (IS_ERR(rockchip->vpcie3v3))
>>>> +	if (!rockchip->vpcie3v3)
>>>>    		return;
>>>>    	/*
>>>> @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>>    		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
>>>>    			return PTR_ERR(rockchip->vpcie12v);
>>>>    		dev_info(dev, "no vpcie12v regulator found\n");
>>>> +		rockchip->vpcie12v = NULL;
>>>
>>> According to the API NULL is a valid regulator.  We don't currently
>>> actually do this but it's storing up surprises if you treat it as
>>> invalid.
>>
>> Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
>> immediately dereferencing its argument without any checks. If we'd rather
>> not bake in that assumption then this patch can happily be ignored.
> 
> I'd suggest we drop this patch.
> 
> "IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
> -4095 inclusive.

Hence the explicit initial "if (IS_ERR(ptr)) ptr = NULL;" condition 
quoted above ;)

But yeah, it was merely an attempt at a minor cosmetic cleanup, so let's 
just forget about it to avoid any possible confusion.

Cheers,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Robin Murphy]

On 18/11/2019 12:39 pm, Andrew Murray wrote:
> On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
>> On 18/11/2019 11:59 am, Mark Brown wrote:
>>> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
>>>> Null checks are both cheaper and more readable than having !IS_ERR()
>>>> splattered everywhere.
>>>
>>>> -	if (IS_ERR(rockchip->vpcie3v3))
>>>> +	if (!rockchip->vpcie3v3)
>>>>    		return;
>>>>    	/*
>>>> @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>>    		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
>>>>    			return PTR_ERR(rockchip->vpcie12v);
>>>>    		dev_info(dev, "no vpcie12v regulator found\n");
>>>> +		rockchip->vpcie12v = NULL;
>>>
>>> According to the API NULL is a valid regulator.  We don't currently
>>> actually do this but it's storing up surprises if you treat it as
>>> invalid.
>>
>> Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
>> immediately dereferencing its argument without any checks. If we'd rather
>> not bake in that assumption then this patch can happily be ignored.
> 
> I'd suggest we drop this patch.
> 
> "IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
> -4095 inclusive.

Hence the explicit initial "if (IS_ERR(ptr)) ptr = NULL;" condition 
quoted above ;)

But yeah, it was merely an attempt at a minor cosmetic cleanup, so let's 
just forget about it to avoid any possible confusion.

Cheers,
Robin.

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Andrew Murray]

On Mon, Nov 18, 2019 at 01:10:58PM +0000, Robin Murphy wrote:
> On 18/11/2019 12:39 pm, Andrew Murray wrote:
> > On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
> > > On 18/11/2019 11:59 am, Mark Brown wrote:
> > > > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > > > splattered everywhere.
> > > > 
> > > > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > > > +	if (!rockchip->vpcie3v3)
> > > > >    		return;
> > > > >    	/*
> > > > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > > > >    		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > > > >    			return PTR_ERR(rockchip->vpcie12v);
> > > > >    		dev_info(dev, "no vpcie12v regulator found\n");
> > > > > +		rockchip->vpcie12v = NULL;
> > > > 
> > > > According to the API NULL is a valid regulator.  We don't currently
> > > > actually do this but it's storing up surprises if you treat it as
> > > > invalid.
> > > 
> > > Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
> > > immediately dereferencing its argument without any checks. If we'd rather
> > > not bake in that assumption then this patch can happily be ignored.
> > 
> > I'd suggest we drop this patch.
> > 
> > "IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
> > -4095 inclusive.
> 
> Hence the explicit initial "if (IS_ERR(ptr)) ptr = NULL;" condition quoted
> above ;)

Doh.

Andrew Murray

> 
> But yeah, it was merely an attempt at a minor cosmetic cleanup, so let's
> just forget about it to avoid any possible confusion.
> 
> Cheers,
> Robin.

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Andrew Murray]

On Mon, Nov 18, 2019 at 01:10:58PM +0000, Robin Murphy wrote:
> On 18/11/2019 12:39 pm, Andrew Murray wrote:
> > On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
> > > On 18/11/2019 11:59 am, Mark Brown wrote:
> > > > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > > > splattered everywhere.
> > > > 
> > > > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > > > +	if (!rockchip->vpcie3v3)
> > > > >    		return;
> > > > >    	/*
> > > > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > > > >    		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > > > >    			return PTR_ERR(rockchip->vpcie12v);
> > > > >    		dev_info(dev, "no vpcie12v regulator found\n");
> > > > > +		rockchip->vpcie12v = NULL;
> > > > 
> > > > According to the API NULL is a valid regulator.  We don't currently
> > > > actually do this but it's storing up surprises if you treat it as
> > > > invalid.
> > > 
> > > Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
> > > immediately dereferencing its argument without any checks. If we'd rather
> > > not bake in that assumption then this patch can happily be ignored.
> > 
> > I'd suggest we drop this patch.
> > 
> > "IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
> > -4095 inclusive.
> 
> Hence the explicit initial "if (IS_ERR(ptr)) ptr = NULL;" condition quoted
> above ;)

Doh.

Andrew Murray

> 
> But yeah, it was merely an attempt at a minor cosmetic cleanup, so let's
> just forget about it to avoid any possible confusion.
> 
> Cheers,
> Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Andrew Murray]

On Mon, Nov 18, 2019 at 01:10:58PM +0000, Robin Murphy wrote:
> On 18/11/2019 12:39 pm, Andrew Murray wrote:
> > On Mon, Nov 18, 2019 at 12:20:10PM +0000, Robin Murphy wrote:
> > > On 18/11/2019 11:59 am, Mark Brown wrote:
> > > > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > > > splattered everywhere.
> > > > 
> > > > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > > > +	if (!rockchip->vpcie3v3)
> > > > >    		return;
> > > > >    	/*
> > > > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > > > >    		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > > > >    			return PTR_ERR(rockchip->vpcie12v);
> > > > >    		dev_info(dev, "no vpcie12v regulator found\n");
> > > > > +		rockchip->vpcie12v = NULL;
> > > > 
> > > > According to the API NULL is a valid regulator.  We don't currently
> > > > actually do this but it's storing up surprises if you treat it as
> > > > invalid.
> > > 
> > > Ah, OK - I'd assumed NULL wasn't valid based on regulator_enable()
> > > immediately dereferencing its argument without any checks. If we'd rather
> > > not bake in that assumption then this patch can happily be ignored.
> > 
> > I'd suggest we drop this patch.
> > 
> > "IS_ERR(ptr)" is not the same as "!ptr", for values of ptr between 0 and
> > -4095 inclusive.
> 
> Hence the explicit initial "if (IS_ERR(ptr)) ptr = NULL;" condition quoted
> above ;)

Doh.

Andrew Murray

> 
> But yeah, it was merely an attempt at a minor cosmetic cleanup, so let's
> just forget about it to avoid any possible confusion.
> 
> Cheers,
> Robin.

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Bjorn Helgaas]

On Mon, Nov 18, 2019 at 11:59:30AM +0000, Mark Brown wrote:
> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > Null checks are both cheaper and more readable than having !IS_ERR()
> > splattered everywhere.
> 
> > -	if (IS_ERR(rockchip->vpcie3v3))
> > +	if (!rockchip->vpcie3v3)
> >  		return;
> >  
> >  	/*
> > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> >  		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> >  			return PTR_ERR(rockchip->vpcie12v);
> >  		dev_info(dev, "no vpcie12v regulator found\n");
> > +		rockchip->vpcie12v = NULL;
> 
> According to the API NULL is a valid regulator.  We don't currently
> actually do this but it's storing up surprises if you treat it as
> invalid.

I don't know anything about the regulator API, but the fact that NULL
can be a valid regulator is itself a little surprising :)

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Bjorn Helgaas]

On Mon, Nov 18, 2019 at 11:59:30AM +0000, Mark Brown wrote:
> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > Null checks are both cheaper and more readable than having !IS_ERR()
> > splattered everywhere.
> 
> > -	if (IS_ERR(rockchip->vpcie3v3))
> > +	if (!rockchip->vpcie3v3)
> >  		return;
> >  
> >  	/*
> > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> >  		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> >  			return PTR_ERR(rockchip->vpcie12v);
> >  		dev_info(dev, "no vpcie12v regulator found\n");
> > +		rockchip->vpcie12v = NULL;
> 
> According to the API NULL is a valid regulator.  We don't currently
> actually do this but it's storing up surprises if you treat it as
> invalid.

I don't know anything about the regulator API, but the fact that NULL
can be a valid regulator is itself a little surprising :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Bjorn Helgaas]

On Mon, Nov 18, 2019 at 11:59:30AM +0000, Mark Brown wrote:
> On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > Null checks are both cheaper and more readable than having !IS_ERR()
> > splattered everywhere.
> 
> > -	if (IS_ERR(rockchip->vpcie3v3))
> > +	if (!rockchip->vpcie3v3)
> >  		return;
> >  
> >  	/*
> > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> >  		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> >  			return PTR_ERR(rockchip->vpcie12v);
> >  		dev_info(dev, "no vpcie12v regulator found\n");
> > +		rockchip->vpcie12v = NULL;
> 
> According to the API NULL is a valid regulator.  We don't currently
> actually do this but it's storing up surprises if you treat it as
> invalid.

I don't know anything about the regulator API, but the fact that NULL
can be a valid regulator is itself a little surprising :)

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Lorenzo Pieralisi]

On Mon, Nov 18, 2019 at 08:24:28AM -0600, Bjorn Helgaas wrote:
> On Mon, Nov 18, 2019 at 11:59:30AM +0000, Mark Brown wrote:
> > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > splattered everywhere.
> > 
> > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > +	if (!rockchip->vpcie3v3)
> > >  		return;
> > >  
> > >  	/*
> > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > >  		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > >  			return PTR_ERR(rockchip->vpcie12v);
> > >  		dev_info(dev, "no vpcie12v regulator found\n");
> > > +		rockchip->vpcie12v = NULL;
> > 
> > According to the API NULL is a valid regulator.  We don't currently
> > actually do this but it's storing up surprises if you treat it as
> > invalid.
> 
> I don't know anything about the regulator API, but the fact that NULL
> can be a valid regulator is itself a little surprising :)

if (rockchip->vpcie3v3 == NULL) is true the driver would currently
panic the kernel AFAICS.

rockchip_pcie_set_power_limit()
->regulator_get_current_limit(NULL)
 -> _regulator_get_current_limit(NULL)
   -> regulator_lock(NULL)
     -> regulator_lock_nested(NULL, NULL)
       -> ww_mutex_trylock(&NULL->mutex)

Also, by making NULL a valid regulator, it means that regulators
(ie pointers) with default values are valid whether we call
devm_regulator_get* or not. I understand this patch can be dropped
but that per-se does not make this driver code any more robust AFAICS.

Lorenzo

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Lorenzo Pieralisi]

On Mon, Nov 18, 2019 at 08:24:28AM -0600, Bjorn Helgaas wrote:
> On Mon, Nov 18, 2019 at 11:59:30AM +0000, Mark Brown wrote:
> > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > splattered everywhere.
> > 
> > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > +	if (!rockchip->vpcie3v3)
> > >  		return;
> > >  
> > >  	/*
> > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > >  		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > >  			return PTR_ERR(rockchip->vpcie12v);
> > >  		dev_info(dev, "no vpcie12v regulator found\n");
> > > +		rockchip->vpcie12v = NULL;
> > 
> > According to the API NULL is a valid regulator.  We don't currently
> > actually do this but it's storing up surprises if you treat it as
> > invalid.
> 
> I don't know anything about the regulator API, but the fact that NULL
> can be a valid regulator is itself a little surprising :)

if (rockchip->vpcie3v3 == NULL) is true the driver would currently
panic the kernel AFAICS.

rockchip_pcie_set_power_limit()
->regulator_get_current_limit(NULL)
 -> _regulator_get_current_limit(NULL)
   -> regulator_lock(NULL)
     -> regulator_lock_nested(NULL, NULL)
       -> ww_mutex_trylock(&NULL->mutex)

Also, by making NULL a valid regulator, it means that regulators
(ie pointers) with default values are valid whether we call
devm_regulator_get* or not. I understand this patch can be dropped
but that per-se does not make this driver code any more robust AFAICS.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Lorenzo Pieralisi]

On Mon, Nov 18, 2019 at 08:24:28AM -0600, Bjorn Helgaas wrote:
> On Mon, Nov 18, 2019 at 11:59:30AM +0000, Mark Brown wrote:
> > On Sat, Nov 16, 2019 at 12:54:20PM +0000, Robin Murphy wrote:
> > > Null checks are both cheaper and more readable than having !IS_ERR()
> > > splattered everywhere.
> > 
> > > -	if (IS_ERR(rockchip->vpcie3v3))
> > > +	if (!rockchip->vpcie3v3)
> > >  		return;
> > >  
> > >  	/*
> > > @@ -611,6 +611,7 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > >  		if (PTR_ERR(rockchip->vpcie12v) != -ENODEV)
> > >  			return PTR_ERR(rockchip->vpcie12v);
> > >  		dev_info(dev, "no vpcie12v regulator found\n");
> > > +		rockchip->vpcie12v = NULL;
> > 
> > According to the API NULL is a valid regulator.  We don't currently
> > actually do this but it's storing up surprises if you treat it as
> > invalid.
> 
> I don't know anything about the regulator API, but the fact that NULL
> can be a valid regulator is itself a little surprising :)

if (rockchip->vpcie3v3 == NULL) is true the driver would currently
panic the kernel AFAICS.

rockchip_pcie_set_power_limit()
->regulator_get_current_limit(NULL)
 -> _regulator_get_current_limit(NULL)
   -> regulator_lock(NULL)
     -> regulator_lock_nested(NULL, NULL)
       -> ww_mutex_trylock(&NULL->mutex)

Also, by making NULL a valid regulator, it means that regulators
(ie pointers) with default values are valid whether we call
devm_regulator_get* or not. I understand this patch can be dropped
but that per-se does not make this driver code any more robust AFAICS.

Lorenzo

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Mark Brown]

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

On Mon, Nov 18, 2019 at 06:15:38PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 18, 2019 at 08:24:28AM -0600, Bjorn Helgaas wrote:

> > I don't know anything about the regulator API, but the fact that NULL
> > can be a valid regulator is itself a little surprising :)

regulator_get() has always been documented as returning a valid
regulator or an ERR_PTR().

> if (rockchip->vpcie3v3 == NULL) is true the driver would currently
> panic the kernel AFAICS.

We don't currently use this, it's just something we could do.

> Also, by making NULL a valid regulator, it means that regulators
> (ie pointers) with default values are valid whether we call
> devm_regulator_get* or not. I understand this patch can be dropped
> but that per-se does not make this driver code any more robust AFAICS.

That's one reason we've not done this.

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

Re: [PATCH 2/2] PCI: rockchip: Simplify optional regulator handling

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 833 bytes --]

On Mon, Nov 18, 2019 at 06:15:38PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 18, 2019 at 08:24:28AM -0600, Bjorn Helgaas wrote:

> > I don't know anything about the regulator API, but the fact that NULL
> > can be a valid regulator is itself a little surprising :)

regulator_get() has always been documented as returning a valid
regulator or an ERR_PTR().

> if (rockchip->vpcie3v3 == NULL) is true the driver would currently
> panic the kernel AFAICS.

We don't currently use this, it's just something we could do.

> Also, by making NULL a valid regulator, it means that regulators
> (ie pointers) with default values are valid whether we call
> devm_regulator_get* or not. I understand this patch can be dropped
> but that per-se does not make this driver code any more robust AFAICS.

That's one reason we've not done this.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Mark Brown]

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

On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> are thus fundamental to PCIe being usable at all. As such, it makes
> sense to treat them as non-optional and rely on dummy regulators if
> not explicitly described.

Reviewed-by: Mark Brown <broonie@kernel.org>

This not only makes sense it's a fix.  regulator_get_optional() should
only be used if the supply may be physically absent (eg, when the
feature can be left unpowered or where there's an option to switch in an
internal regualtor).  

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

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 593 bytes --]

On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> are thus fundamental to PCIe being usable at all. As such, it makes
> sense to treat them as non-optional and rely on dummy regulators if
> not explicitly described.

Reviewed-by: Mark Brown <broonie@kernel.org>

This not only makes sense it's a fix.  regulator_get_optional() should
only be used if the supply may be physically absent (eg, when the
feature can be left unpowered or where there's an option to switch in an
internal regualtor).  

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Andrew Murray]

On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> are thus fundamental to PCIe being usable at all. As such, it makes
> sense to treat them as non-optional and rely on dummy regulators if
> not explicitly described.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

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

> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>  1 file changed, 25 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index ef8e677ce9d1..68525f8ac4d9 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>  	}
>  
> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
> -	if (IS_ERR(rockchip->vpcie1v8)) {
> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie1v8);
> -		dev_info(dev, "no vpcie1v8 regulator found\n");
> -	}
> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
> +	if (IS_ERR(rockchip->vpcie1v8))
> +		return PTR_ERR(rockchip->vpcie1v8);
>  
> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
> -	if (IS_ERR(rockchip->vpcie0v9)) {
> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie0v9);
> -		dev_info(dev, "no vpcie0v9 regulator found\n");
> -	}
> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
> +	if (IS_ERR(rockchip->vpcie0v9))
> +		return PTR_ERR(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>  		}
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie1v8)) {
> -		err = regulator_enable(rockchip->vpcie1v8);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> -			goto err_disable_3v3;
> -		}
> +	err = regulator_enable(rockchip->vpcie1v8);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> +		goto err_disable_3v3;
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			goto err_disable_1v8;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		goto err_disable_1v8;
>  	}
>  
>  	return 0;
>  
>  err_disable_1v8:
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie1v8);
>  err_disable_3v3:
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>  
>  	rockchip_pcie_disable_clocks(rockchip);
>  
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return ret;
>  }
> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>  	int err;
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			return err;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		return err;
>  	}
>  
>  	err = rockchip_pcie_enable_clocks(rockchip);
> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  err_pcie_resume:
>  	rockchip_pcie_disable_clocks(rockchip);
>  err_disable_0v9:
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  	return err;
>  }
>  
> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  err_set_vpcie:
>  	rockchip_pcie_disable_clocks(rockchip);
>  	return err;
> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Andrew Murray]

On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> are thus fundamental to PCIe being usable at all. As such, it makes
> sense to treat them as non-optional and rely on dummy regulators if
> not explicitly described.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

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

> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>  1 file changed, 25 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index ef8e677ce9d1..68525f8ac4d9 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>  	}
>  
> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
> -	if (IS_ERR(rockchip->vpcie1v8)) {
> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie1v8);
> -		dev_info(dev, "no vpcie1v8 regulator found\n");
> -	}
> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
> +	if (IS_ERR(rockchip->vpcie1v8))
> +		return PTR_ERR(rockchip->vpcie1v8);
>  
> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
> -	if (IS_ERR(rockchip->vpcie0v9)) {
> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie0v9);
> -		dev_info(dev, "no vpcie0v9 regulator found\n");
> -	}
> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
> +	if (IS_ERR(rockchip->vpcie0v9))
> +		return PTR_ERR(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>  		}
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie1v8)) {
> -		err = regulator_enable(rockchip->vpcie1v8);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> -			goto err_disable_3v3;
> -		}
> +	err = regulator_enable(rockchip->vpcie1v8);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> +		goto err_disable_3v3;
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			goto err_disable_1v8;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		goto err_disable_1v8;
>  	}
>  
>  	return 0;
>  
>  err_disable_1v8:
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie1v8);
>  err_disable_3v3:
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>  
>  	rockchip_pcie_disable_clocks(rockchip);
>  
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return ret;
>  }
> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>  	int err;
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			return err;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		return err;
>  	}
>  
>  	err = rockchip_pcie_enable_clocks(rockchip);
> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  err_pcie_resume:
>  	rockchip_pcie_disable_clocks(rockchip);
>  err_disable_0v9:
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  	return err;
>  }
>  
> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  err_set_vpcie:
>  	rockchip_pcie_disable_clocks(rockchip);
>  	return err;
> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Andrew Murray]

On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> are thus fundamental to PCIe being usable at all. As such, it makes
> sense to treat them as non-optional and rely on dummy regulators if
> not explicitly described.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

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

> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>  1 file changed, 25 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index ef8e677ce9d1..68525f8ac4d9 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>  	}
>  
> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
> -	if (IS_ERR(rockchip->vpcie1v8)) {
> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie1v8);
> -		dev_info(dev, "no vpcie1v8 regulator found\n");
> -	}
> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
> +	if (IS_ERR(rockchip->vpcie1v8))
> +		return PTR_ERR(rockchip->vpcie1v8);
>  
> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
> -	if (IS_ERR(rockchip->vpcie0v9)) {
> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie0v9);
> -		dev_info(dev, "no vpcie0v9 regulator found\n");
> -	}
> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
> +	if (IS_ERR(rockchip->vpcie0v9))
> +		return PTR_ERR(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>  		}
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie1v8)) {
> -		err = regulator_enable(rockchip->vpcie1v8);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> -			goto err_disable_3v3;
> -		}
> +	err = regulator_enable(rockchip->vpcie1v8);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> +		goto err_disable_3v3;
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			goto err_disable_1v8;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		goto err_disable_1v8;
>  	}
>  
>  	return 0;
>  
>  err_disable_1v8:
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie1v8);
>  err_disable_3v3:
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>  
>  	rockchip_pcie_disable_clocks(rockchip);
>  
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return ret;
>  }
> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>  	int err;
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			return err;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		return err;
>  	}
>  
>  	err = rockchip_pcie_enable_clocks(rockchip);
> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  err_pcie_resume:
>  	rockchip_pcie_disable_clocks(rockchip);
>  err_disable_0v9:
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  	return err;
>  }
>  
> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  err_set_vpcie:
>  	rockchip_pcie_disable_clocks(rockchip);
>  	return err;
> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Lorenzo Pieralisi]

On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> are thus fundamental to PCIe being usable at all. As such, it makes
> sense to treat them as non-optional and rely on dummy regulators if
> not explicitly described.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>  1 file changed, 25 insertions(+), 44 deletions(-)

Applied to pci/rockchip, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index ef8e677ce9d1..68525f8ac4d9 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>  	}
>  
> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
> -	if (IS_ERR(rockchip->vpcie1v8)) {
> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie1v8);
> -		dev_info(dev, "no vpcie1v8 regulator found\n");
> -	}
> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
> +	if (IS_ERR(rockchip->vpcie1v8))
> +		return PTR_ERR(rockchip->vpcie1v8);
>  
> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
> -	if (IS_ERR(rockchip->vpcie0v9)) {
> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie0v9);
> -		dev_info(dev, "no vpcie0v9 regulator found\n");
> -	}
> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
> +	if (IS_ERR(rockchip->vpcie0v9))
> +		return PTR_ERR(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>  		}
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie1v8)) {
> -		err = regulator_enable(rockchip->vpcie1v8);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> -			goto err_disable_3v3;
> -		}
> +	err = regulator_enable(rockchip->vpcie1v8);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> +		goto err_disable_3v3;
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			goto err_disable_1v8;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		goto err_disable_1v8;
>  	}
>  
>  	return 0;
>  
>  err_disable_1v8:
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie1v8);
>  err_disable_3v3:
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>  
>  	rockchip_pcie_disable_clocks(rockchip);
>  
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return ret;
>  }
> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>  	int err;
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			return err;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		return err;
>  	}
>  
>  	err = rockchip_pcie_enable_clocks(rockchip);
> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  err_pcie_resume:
>  	rockchip_pcie_disable_clocks(rockchip);
>  err_disable_0v9:
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  	return err;
>  }
>  
> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  err_set_vpcie:
>  	rockchip_pcie_disable_clocks(rockchip);
>  	return err;
> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Lorenzo Pieralisi]

On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> are thus fundamental to PCIe being usable at all. As such, it makes
> sense to treat them as non-optional and rely on dummy regulators if
> not explicitly described.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>  1 file changed, 25 insertions(+), 44 deletions(-)

Applied to pci/rockchip, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index ef8e677ce9d1..68525f8ac4d9 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>  	}
>  
> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
> -	if (IS_ERR(rockchip->vpcie1v8)) {
> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie1v8);
> -		dev_info(dev, "no vpcie1v8 regulator found\n");
> -	}
> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
> +	if (IS_ERR(rockchip->vpcie1v8))
> +		return PTR_ERR(rockchip->vpcie1v8);
>  
> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
> -	if (IS_ERR(rockchip->vpcie0v9)) {
> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie0v9);
> -		dev_info(dev, "no vpcie0v9 regulator found\n");
> -	}
> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
> +	if (IS_ERR(rockchip->vpcie0v9))
> +		return PTR_ERR(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>  		}
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie1v8)) {
> -		err = regulator_enable(rockchip->vpcie1v8);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> -			goto err_disable_3v3;
> -		}
> +	err = regulator_enable(rockchip->vpcie1v8);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> +		goto err_disable_3v3;
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			goto err_disable_1v8;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		goto err_disable_1v8;
>  	}
>  
>  	return 0;
>  
>  err_disable_1v8:
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie1v8);
>  err_disable_3v3:
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>  
>  	rockchip_pcie_disable_clocks(rockchip);
>  
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return ret;
>  }
> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>  	int err;
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			return err;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		return err;
>  	}
>  
>  	err = rockchip_pcie_enable_clocks(rockchip);
> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  err_pcie_resume:
>  	rockchip_pcie_disable_clocks(rockchip);
>  err_disable_0v9:
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  	return err;
>  }
>  
> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  err_set_vpcie:
>  	rockchip_pcie_disable_clocks(rockchip);
>  	return err;
> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Lorenzo Pieralisi]

On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> are thus fundamental to PCIe being usable at all. As such, it makes
> sense to treat them as non-optional and rely on dummy regulators if
> not explicitly described.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>  1 file changed, 25 insertions(+), 44 deletions(-)

Applied to pci/rockchip, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index ef8e677ce9d1..68525f8ac4d9 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>  	}
>  
> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
> -	if (IS_ERR(rockchip->vpcie1v8)) {
> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie1v8);
> -		dev_info(dev, "no vpcie1v8 regulator found\n");
> -	}
> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
> +	if (IS_ERR(rockchip->vpcie1v8))
> +		return PTR_ERR(rockchip->vpcie1v8);
>  
> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
> -	if (IS_ERR(rockchip->vpcie0v9)) {
> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> -			return PTR_ERR(rockchip->vpcie0v9);
> -		dev_info(dev, "no vpcie0v9 regulator found\n");
> -	}
> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
> +	if (IS_ERR(rockchip->vpcie0v9))
> +		return PTR_ERR(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>  		}
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie1v8)) {
> -		err = regulator_enable(rockchip->vpcie1v8);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> -			goto err_disable_3v3;
> -		}
> +	err = regulator_enable(rockchip->vpcie1v8);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> +		goto err_disable_3v3;
>  	}
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			goto err_disable_1v8;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		goto err_disable_1v8;
>  	}
>  
>  	return 0;
>  
>  err_disable_1v8:
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie1v8);
>  err_disable_3v3:
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>  
>  	rockchip_pcie_disable_clocks(rockchip);
>  
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return ret;
>  }
> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>  	int err;
>  
> -	if (!IS_ERR(rockchip->vpcie0v9)) {
> -		err = regulator_enable(rockchip->vpcie0v9);
> -		if (err) {
> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> -			return err;
> -		}
> +	err = regulator_enable(rockchip->vpcie0v9);
> +	if (err) {
> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> +		return err;
>  	}
>  
>  	err = rockchip_pcie_enable_clocks(rockchip);
> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>  err_pcie_resume:
>  	rockchip_pcie_disable_clocks(rockchip);
>  err_disable_0v9:
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie0v9);
>  	return err;
>  }
>  
> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  err_set_vpcie:
>  	rockchip_pcie_disable_clocks(rockchip);
>  	return err;
> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>  		regulator_disable(rockchip->vpcie12v);
>  	if (!IS_ERR(rockchip->vpcie3v3))
>  		regulator_disable(rockchip->vpcie3v3);
> -	if (!IS_ERR(rockchip->vpcie1v8))
> -		regulator_disable(rockchip->vpcie1v8);
> -	if (!IS_ERR(rockchip->vpcie0v9))
> -		regulator_disable(rockchip->vpcie0v9);
> +	regulator_disable(rockchip->vpcie1v8);
> +	regulator_disable(rockchip->vpcie0v9);
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>> are thus fundamental to PCIe being usable at all. As such, it makes
>> sense to treat them as non-optional and rely on dummy regulators if
>> not explicitly described.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>  1 file changed, 25 insertions(+), 44 deletions(-)
> 
> Applied to pci/rockchip, thanks.

Sorry, this commit is cause regression for RK3399 boards unable to
detect the controller anymore.

The 1v8 (and 0v9) is causing -517 and reject the controller initialization.

I'm not a PCI guy, but a quick google search shows these two voltages
are not related to PCIE core functionality, especially considering the
controller used in RK3399 are mostly to provide NVME support.

This bug makes all RK3399 users who put root fs into NVME driver unable
to boot the device.

I really hope some one could test the patch before affecting the end
users or at least try to understand how most users would use the PCIE
interface for.

Thanks,
Qu

> 
> Lorenzo
> 
>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>> index ef8e677ce9d1..68525f8ac4d9 100644
>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>>  	}
>>  
>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>> -			return PTR_ERR(rockchip->vpcie1v8);
>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>> -	}
>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>> +	if (IS_ERR(rockchip->vpcie1v8))
>> +		return PTR_ERR(rockchip->vpcie1v8);
>>  
>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>> -			return PTR_ERR(rockchip->vpcie0v9);
>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>> -	}
>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>> +	if (IS_ERR(rockchip->vpcie0v9))
>> +		return PTR_ERR(rockchip->vpcie0v9);
>>  
>>  	return 0;
>>  }
>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>  		}
>>  	}
>>  
>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>> -		err = regulator_enable(rockchip->vpcie1v8);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>> -			goto err_disable_3v3;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie1v8);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>> +		goto err_disable_3v3;
>>  	}
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>> -		err = regulator_enable(rockchip->vpcie0v9);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> -			goto err_disable_1v8;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie0v9);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> +		goto err_disable_1v8;
>>  	}
>>  
>>  	return 0;
>>  
>>  err_disable_1v8:
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie1v8);
>>  err_disable_3v3:
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>  
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  
>>  	return ret;
>>  }
>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>  	int err;
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>> -		err = regulator_enable(rockchip->vpcie0v9);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> -			return err;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie0v9);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> +		return err;
>>  	}
>>  
>>  	err = rockchip_pcie_enable_clocks(rockchip);
>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>  err_pcie_resume:
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  err_disable_0v9:
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  	return err;
>>  }
>>  
>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>  		regulator_disable(rockchip->vpcie12v);
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  err_set_vpcie:
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  	return err;
>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>  		regulator_disable(rockchip->vpcie12v);
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.17.1
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>> are thus fundamental to PCIe being usable at all. As such, it makes
>> sense to treat them as non-optional and rely on dummy regulators if
>> not explicitly described.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>  1 file changed, 25 insertions(+), 44 deletions(-)
> 
> Applied to pci/rockchip, thanks.

Sorry, this commit is cause regression for RK3399 boards unable to
detect the controller anymore.

The 1v8 (and 0v9) is causing -517 and reject the controller initialization.

I'm not a PCI guy, but a quick google search shows these two voltages
are not related to PCIE core functionality, especially considering the
controller used in RK3399 are mostly to provide NVME support.

This bug makes all RK3399 users who put root fs into NVME driver unable
to boot the device.

I really hope some one could test the patch before affecting the end
users or at least try to understand how most users would use the PCIE
interface for.

Thanks,
Qu

> 
> Lorenzo
> 
>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>> index ef8e677ce9d1..68525f8ac4d9 100644
>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>>  	}
>>  
>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>> -			return PTR_ERR(rockchip->vpcie1v8);
>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>> -	}
>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>> +	if (IS_ERR(rockchip->vpcie1v8))
>> +		return PTR_ERR(rockchip->vpcie1v8);
>>  
>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>> -			return PTR_ERR(rockchip->vpcie0v9);
>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>> -	}
>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>> +	if (IS_ERR(rockchip->vpcie0v9))
>> +		return PTR_ERR(rockchip->vpcie0v9);
>>  
>>  	return 0;
>>  }
>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>  		}
>>  	}
>>  
>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>> -		err = regulator_enable(rockchip->vpcie1v8);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>> -			goto err_disable_3v3;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie1v8);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>> +		goto err_disable_3v3;
>>  	}
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>> -		err = regulator_enable(rockchip->vpcie0v9);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> -			goto err_disable_1v8;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie0v9);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> +		goto err_disable_1v8;
>>  	}
>>  
>>  	return 0;
>>  
>>  err_disable_1v8:
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie1v8);
>>  err_disable_3v3:
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>  
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  
>>  	return ret;
>>  }
>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>  	int err;
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>> -		err = regulator_enable(rockchip->vpcie0v9);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> -			return err;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie0v9);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> +		return err;
>>  	}
>>  
>>  	err = rockchip_pcie_enable_clocks(rockchip);
>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>  err_pcie_resume:
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  err_disable_0v9:
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  	return err;
>>  }
>>  
>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>  		regulator_disable(rockchip->vpcie12v);
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  err_set_vpcie:
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  	return err;
>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>  		regulator_disable(rockchip->vpcie12v);
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.17.1
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>> are thus fundamental to PCIe being usable at all. As such, it makes
>> sense to treat them as non-optional and rely on dummy regulators if
>> not explicitly described.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>  1 file changed, 25 insertions(+), 44 deletions(-)
> 
> Applied to pci/rockchip, thanks.

Sorry, this commit is cause regression for RK3399 boards unable to
detect the controller anymore.

The 1v8 (and 0v9) is causing -517 and reject the controller initialization.

I'm not a PCI guy, but a quick google search shows these two voltages
are not related to PCIE core functionality, especially considering the
controller used in RK3399 are mostly to provide NVME support.

This bug makes all RK3399 users who put root fs into NVME driver unable
to boot the device.

I really hope some one could test the patch before affecting the end
users or at least try to understand how most users would use the PCIE
interface for.

Thanks,
Qu

> 
> Lorenzo
> 
>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>> index ef8e677ce9d1..68525f8ac4d9 100644
>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>>  	}
>>  
>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>> -			return PTR_ERR(rockchip->vpcie1v8);
>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>> -	}
>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>> +	if (IS_ERR(rockchip->vpcie1v8))
>> +		return PTR_ERR(rockchip->vpcie1v8);
>>  
>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>> -			return PTR_ERR(rockchip->vpcie0v9);
>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>> -	}
>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>> +	if (IS_ERR(rockchip->vpcie0v9))
>> +		return PTR_ERR(rockchip->vpcie0v9);
>>  
>>  	return 0;
>>  }
>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>  		}
>>  	}
>>  
>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>> -		err = regulator_enable(rockchip->vpcie1v8);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>> -			goto err_disable_3v3;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie1v8);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>> +		goto err_disable_3v3;
>>  	}
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>> -		err = regulator_enable(rockchip->vpcie0v9);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> -			goto err_disable_1v8;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie0v9);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> +		goto err_disable_1v8;
>>  	}
>>  
>>  	return 0;
>>  
>>  err_disable_1v8:
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie1v8);
>>  err_disable_3v3:
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>  
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  
>>  	return ret;
>>  }
>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>  	int err;
>>  
>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>> -		err = regulator_enable(rockchip->vpcie0v9);
>> -		if (err) {
>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> -			return err;
>> -		}
>> +	err = regulator_enable(rockchip->vpcie0v9);
>> +	if (err) {
>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>> +		return err;
>>  	}
>>  
>>  	err = rockchip_pcie_enable_clocks(rockchip);
>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>  err_pcie_resume:
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  err_disable_0v9:
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  	return err;
>>  }
>>  
>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>  		regulator_disable(rockchip->vpcie12v);
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  err_set_vpcie:
>>  	rockchip_pcie_disable_clocks(rockchip);
>>  	return err;
>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>  		regulator_disable(rockchip->vpcie12v);
>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>  		regulator_disable(rockchip->vpcie3v3);
>> -	if (!IS_ERR(rockchip->vpcie1v8))
>> -		regulator_disable(rockchip->vpcie1v8);
>> -	if (!IS_ERR(rockchip->vpcie0v9))
>> -		regulator_disable(rockchip->vpcie0v9);
>> +	regulator_disable(rockchip->vpcie1v8);
>> +	regulator_disable(rockchip->vpcie0v9);
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.17.1
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午7:36, Qu Wenruo wrote:
> 
> 
> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>> sense to treat them as non-optional and rely on dummy regulators if
>>> not explicitly described.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>>  1 file changed, 25 insertions(+), 44 deletions(-)
>>
>> Applied to pci/rockchip, thanks.
> 
> Sorry, this commit is cause regression for RK3399 boards unable to
> detect the controller anymore.
> 
> The 1v8 (and 0v9) is causing -517 and reject the controller initialization.
> 
> I'm not a PCI guy, but a quick google search shows these two voltages
> are not related to PCIE core functionality, especially considering the
> controller used in RK3399 are mostly to provide NVME support.
> 
> This bug makes all RK3399 users who put root fs into NVME driver unable
> to boot the device.
> 
> I really hope some one could test the patch before affecting the end
> users or at least try to understand how most users would use the PCIE
> interface for.

My bad, it's not that easy. The dtsi has vpcie1v8 and vpcie0v9 defined.
It should be something else wrong.

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>>
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>>>  	}
>>>  
>>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie1v8);
>>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>> +	if (IS_ERR(rockchip->vpcie1v8))
>>> +		return PTR_ERR(rockchip->vpcie1v8);
>>>  
>>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie0v9);
>>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>> +	if (IS_ERR(rockchip->vpcie0v9))
>>> +		return PTR_ERR(rockchip->vpcie0v9);
>>>  
>>>  	return 0;
>>>  }
>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>>  		}
>>>  	}
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>>> -		err = regulator_enable(rockchip->vpcie1v8);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> -			goto err_disable_3v3;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie1v8);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> +		goto err_disable_3v3;
>>>  	}
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			goto err_disable_1v8;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		goto err_disable_1v8;
>>>  	}
>>>  
>>>  	return 0;
>>>  
>>>  err_disable_1v8:
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>>  err_disable_3v3:
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>>  
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  
>>>  	return ret;
>>>  }
>>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>  	int err;
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			return err;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		return err;
>>>  	}
>>>  
>>>  	err = rockchip_pcie_enable_clocks(rockchip);
>>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>  err_pcie_resume:
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  err_disable_0v9:
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  	return err;
>>>  }
>>>  
>>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>  		regulator_disable(rockchip->vpcie12v);
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  err_set_vpcie:
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  	return err;
>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>>  		regulator_disable(rockchip->vpcie12v);
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  
>>>  	return 0;
>>>  }
>>> -- 
>>> 2.17.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午7:36, Qu Wenruo wrote:
> 
> 
> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>> sense to treat them as non-optional and rely on dummy regulators if
>>> not explicitly described.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>>  1 file changed, 25 insertions(+), 44 deletions(-)
>>
>> Applied to pci/rockchip, thanks.
> 
> Sorry, this commit is cause regression for RK3399 boards unable to
> detect the controller anymore.
> 
> The 1v8 (and 0v9) is causing -517 and reject the controller initialization.
> 
> I'm not a PCI guy, but a quick google search shows these two voltages
> are not related to PCIE core functionality, especially considering the
> controller used in RK3399 are mostly to provide NVME support.
> 
> This bug makes all RK3399 users who put root fs into NVME driver unable
> to boot the device.
> 
> I really hope some one could test the patch before affecting the end
> users or at least try to understand how most users would use the PCIE
> interface for.

My bad, it's not that easy. The dtsi has vpcie1v8 and vpcie0v9 defined.
It should be something else wrong.

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>>
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>>>  	}
>>>  
>>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie1v8);
>>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>> +	if (IS_ERR(rockchip->vpcie1v8))
>>> +		return PTR_ERR(rockchip->vpcie1v8);
>>>  
>>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie0v9);
>>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>> +	if (IS_ERR(rockchip->vpcie0v9))
>>> +		return PTR_ERR(rockchip->vpcie0v9);
>>>  
>>>  	return 0;
>>>  }
>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>>  		}
>>>  	}
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>>> -		err = regulator_enable(rockchip->vpcie1v8);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> -			goto err_disable_3v3;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie1v8);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> +		goto err_disable_3v3;
>>>  	}
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			goto err_disable_1v8;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		goto err_disable_1v8;
>>>  	}
>>>  
>>>  	return 0;
>>>  
>>>  err_disable_1v8:
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>>  err_disable_3v3:
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>>  
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  
>>>  	return ret;
>>>  }
>>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>  	int err;
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			return err;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		return err;
>>>  	}
>>>  
>>>  	err = rockchip_pcie_enable_clocks(rockchip);
>>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>  err_pcie_resume:
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  err_disable_0v9:
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  	return err;
>>>  }
>>>  
>>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>  		regulator_disable(rockchip->vpcie12v);
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  err_set_vpcie:
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  	return err;
>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>>  		regulator_disable(rockchip->vpcie12v);
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  
>>>  	return 0;
>>>  }
>>> -- 
>>> 2.17.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午7:36, Qu Wenruo wrote:
> 
> 
> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>> sense to treat them as non-optional and rely on dummy regulators if
>>> not explicitly described.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>  drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>>  1 file changed, 25 insertions(+), 44 deletions(-)
>>
>> Applied to pci/rockchip, thanks.
> 
> Sorry, this commit is cause regression for RK3399 boards unable to
> detect the controller anymore.
> 
> The 1v8 (and 0v9) is causing -517 and reject the controller initialization.
> 
> I'm not a PCI guy, but a quick google search shows these two voltages
> are not related to PCIE core functionality, especially considering the
> controller used in RK3399 are mostly to provide NVME support.
> 
> This bug makes all RK3399 users who put root fs into NVME driver unable
> to boot the device.
> 
> I really hope some one could test the patch before affecting the end
> users or at least try to understand how most users would use the PCIE
> interface for.

My bad, it's not that easy. The dtsi has vpcie1v8 and vpcie0v9 defined.
It should be something else wrong.

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>>
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>  		dev_info(dev, "no vpcie3v3 regulator found\n");
>>>  	}
>>>  
>>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie1v8);
>>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>> +	if (IS_ERR(rockchip->vpcie1v8))
>>> +		return PTR_ERR(rockchip->vpcie1v8);
>>>  
>>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie0v9);
>>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>> +	if (IS_ERR(rockchip->vpcie0v9))
>>> +		return PTR_ERR(rockchip->vpcie0v9);
>>>  
>>>  	return 0;
>>>  }
>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>>  		}
>>>  	}
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>>> -		err = regulator_enable(rockchip->vpcie1v8);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> -			goto err_disable_3v3;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie1v8);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> +		goto err_disable_3v3;
>>>  	}
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			goto err_disable_1v8;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		goto err_disable_1v8;
>>>  	}
>>>  
>>>  	return 0;
>>>  
>>>  err_disable_1v8:
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>>  err_disable_3v3:
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>>  
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  
>>>  	return ret;
>>>  }
>>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>  	int err;
>>>  
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			return err;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		return err;
>>>  	}
>>>  
>>>  	err = rockchip_pcie_enable_clocks(rockchip);
>>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>  err_pcie_resume:
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  err_disable_0v9:
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  	return err;
>>>  }
>>>  
>>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>  		regulator_disable(rockchip->vpcie12v);
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  err_set_vpcie:
>>>  	rockchip_pcie_disable_clocks(rockchip);
>>>  	return err;
>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>>  		regulator_disable(rockchip->vpcie12v);
>>>  	if (!IS_ERR(rockchip->vpcie3v3))
>>>  		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>  
>>>  	return 0;
>>>  }
>>> -- 
>>> 2.17.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

On 2020-11-07 11:36, Qu Wenruo wrote:
> 
> 
> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>> sense to treat them as non-optional and rely on dummy regulators if
>>> not explicitly described.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>   drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>
>> Applied to pci/rockchip, thanks.
> 
> Sorry, this commit is cause regression for RK3399 boards unable to
> detect the controller anymore.
> 
> The 1v8 (and 0v9) is causing -517 and reject the controller initialization.

That's -EPROBE_DEFER, which must mean that a regulator *is* described, 
but you're missing the relevant driver - that's an issue with your 
config/initrd. Being optional should only change the behaviour if the 
supply is totally absent (i.e. you get -ENODEV instead of a dummy 
regulator), so I don't see that it would make any difference in this 
situation anyway :/

> I'm not a PCI guy, but a quick google search shows these two voltages
> are not related to PCIE core functionality, especially considering the
> controller used in RK3399 are mostly to provide NVME support.

Unlike the 12V and 3V3 supplies to the slot, these supplies are to the 
PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the 
datasheet describe as "Supply voltage for PCIE". Having power is kind of 
important for the I/O circuits on all the signal pins to work.

Now it's almost certainly true that these supplies technically belong to 
the phy rather than the controller, but it's a bit late to change the 
bindings for the sake of semantics.

> This bug makes all RK3399 users who put root fs into NVME driver unable
> to boot the device.
> 
> I really hope some one could test the patch before affecting the end
> users or at least try to understand how most users would use the PCIE
> interface for.

I *am* that end user in this case - I use an M.2 NVME on my board, which 
prompted me to take a look at the regulator handling here in the first 
place, to see if it might be possible to shut up the annoying message 
about a 12V supply that is entirely irrelevant to a board without a 
full-size PCIe slot. I use a mainline-based distro, so I've been running 
this change for nearly a year since it landed in v5.5, and I'm sure many 
others have too. I've not heard of any other complaints in that time...

Robin.

> 
> Thanks,
> Qu
> 
>>
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>   		dev_info(dev, "no vpcie3v3 regulator found\n");
>>>   	}
>>>   
>>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie1v8);
>>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>> +	if (IS_ERR(rockchip->vpcie1v8))
>>> +		return PTR_ERR(rockchip->vpcie1v8);
>>>   
>>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie0v9);
>>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>> +	if (IS_ERR(rockchip->vpcie0v9))
>>> +		return PTR_ERR(rockchip->vpcie0v9);
>>>   
>>>   	return 0;
>>>   }
>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>>   		}
>>>   	}
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>>> -		err = regulator_enable(rockchip->vpcie1v8);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> -			goto err_disable_3v3;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie1v8);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> +		goto err_disable_3v3;
>>>   	}
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			goto err_disable_1v8;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		goto err_disable_1v8;
>>>   	}
>>>   
>>>   	return 0;
>>>   
>>>   err_disable_1v8:
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>>   err_disable_3v3:
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>>   
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   
>>>   	return ret;
>>>   }
>>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>   	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>   	int err;
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			return err;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		return err;
>>>   	}
>>>   
>>>   	err = rockchip_pcie_enable_clocks(rockchip);
>>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>   err_pcie_resume:
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   err_disable_0v9:
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   	return err;
>>>   }
>>>   
>>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>   		regulator_disable(rockchip->vpcie12v);
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   err_set_vpcie:
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   	return err;
>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>>   		regulator_disable(rockchip->vpcie12v);
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   
>>>   	return 0;
>>>   }
>>> -- 
>>> 2.17.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

On 2020-11-07 11:36, Qu Wenruo wrote:
> 
> 
> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>> sense to treat them as non-optional and rely on dummy regulators if
>>> not explicitly described.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>   drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>
>> Applied to pci/rockchip, thanks.
> 
> Sorry, this commit is cause regression for RK3399 boards unable to
> detect the controller anymore.
> 
> The 1v8 (and 0v9) is causing -517 and reject the controller initialization.

That's -EPROBE_DEFER, which must mean that a regulator *is* described, 
but you're missing the relevant driver - that's an issue with your 
config/initrd. Being optional should only change the behaviour if the 
supply is totally absent (i.e. you get -ENODEV instead of a dummy 
regulator), so I don't see that it would make any difference in this 
situation anyway :/

> I'm not a PCI guy, but a quick google search shows these two voltages
> are not related to PCIE core functionality, especially considering the
> controller used in RK3399 are mostly to provide NVME support.

Unlike the 12V and 3V3 supplies to the slot, these supplies are to the 
PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the 
datasheet describe as "Supply voltage for PCIE". Having power is kind of 
important for the I/O circuits on all the signal pins to work.

Now it's almost certainly true that these supplies technically belong to 
the phy rather than the controller, but it's a bit late to change the 
bindings for the sake of semantics.

> This bug makes all RK3399 users who put root fs into NVME driver unable
> to boot the device.
> 
> I really hope some one could test the patch before affecting the end
> users or at least try to understand how most users would use the PCIE
> interface for.

I *am* that end user in this case - I use an M.2 NVME on my board, which 
prompted me to take a look at the regulator handling here in the first 
place, to see if it might be possible to shut up the annoying message 
about a 12V supply that is entirely irrelevant to a board without a 
full-size PCIe slot. I use a mainline-based distro, so I've been running 
this change for nearly a year since it landed in v5.5, and I'm sure many 
others have too. I've not heard of any other complaints in that time...

Robin.

> 
> Thanks,
> Qu
> 
>>
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>   		dev_info(dev, "no vpcie3v3 regulator found\n");
>>>   	}
>>>   
>>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie1v8);
>>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>> +	if (IS_ERR(rockchip->vpcie1v8))
>>> +		return PTR_ERR(rockchip->vpcie1v8);
>>>   
>>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie0v9);
>>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>> +	if (IS_ERR(rockchip->vpcie0v9))
>>> +		return PTR_ERR(rockchip->vpcie0v9);
>>>   
>>>   	return 0;
>>>   }
>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>>   		}
>>>   	}
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>>> -		err = regulator_enable(rockchip->vpcie1v8);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> -			goto err_disable_3v3;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie1v8);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> +		goto err_disable_3v3;
>>>   	}
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			goto err_disable_1v8;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		goto err_disable_1v8;
>>>   	}
>>>   
>>>   	return 0;
>>>   
>>>   err_disable_1v8:
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>>   err_disable_3v3:
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>>   
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   
>>>   	return ret;
>>>   }
>>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>   	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>   	int err;
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			return err;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		return err;
>>>   	}
>>>   
>>>   	err = rockchip_pcie_enable_clocks(rockchip);
>>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>   err_pcie_resume:
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   err_disable_0v9:
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   	return err;
>>>   }
>>>   
>>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>   		regulator_disable(rockchip->vpcie12v);
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   err_set_vpcie:
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   	return err;
>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>>   		regulator_disable(rockchip->vpcie12v);
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   
>>>   	return 0;
>>>   }
>>> -- 
>>> 2.17.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

On 2020-11-07 11:36, Qu Wenruo wrote:
> 
> 
> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>> sense to treat them as non-optional and rely on dummy regulators if
>>> not explicitly described.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>   drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>
>> Applied to pci/rockchip, thanks.
> 
> Sorry, this commit is cause regression for RK3399 boards unable to
> detect the controller anymore.
> 
> The 1v8 (and 0v9) is causing -517 and reject the controller initialization.

That's -EPROBE_DEFER, which must mean that a regulator *is* described, 
but you're missing the relevant driver - that's an issue with your 
config/initrd. Being optional should only change the behaviour if the 
supply is totally absent (i.e. you get -ENODEV instead of a dummy 
regulator), so I don't see that it would make any difference in this 
situation anyway :/

> I'm not a PCI guy, but a quick google search shows these two voltages
> are not related to PCIE core functionality, especially considering the
> controller used in RK3399 are mostly to provide NVME support.

Unlike the 12V and 3V3 supplies to the slot, these supplies are to the 
PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the 
datasheet describe as "Supply voltage for PCIE". Having power is kind of 
important for the I/O circuits on all the signal pins to work.

Now it's almost certainly true that these supplies technically belong to 
the phy rather than the controller, but it's a bit late to change the 
bindings for the sake of semantics.

> This bug makes all RK3399 users who put root fs into NVME driver unable
> to boot the device.
> 
> I really hope some one could test the patch before affecting the end
> users or at least try to understand how most users would use the PCIE
> interface for.

I *am* that end user in this case - I use an M.2 NVME on my board, which 
prompted me to take a look at the regulator handling here in the first 
place, to see if it might be possible to shut up the annoying message 
about a 12V supply that is entirely irrelevant to a board without a 
full-size PCIe slot. I use a mainline-based distro, so I've been running 
this change for nearly a year since it landed in v5.5, and I'm sure many 
others have too. I've not heard of any other complaints in that time...

Robin.

> 
> Thanks,
> Qu
> 
>>
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>>   		dev_info(dev, "no vpcie3v3 regulator found\n");
>>>   	}
>>>   
>>> -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>>> -	if (IS_ERR(rockchip->vpcie1v8)) {
>>> -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie1v8);
>>> -		dev_info(dev, "no vpcie1v8 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>> +	if (IS_ERR(rockchip->vpcie1v8))
>>> +		return PTR_ERR(rockchip->vpcie1v8);
>>>   
>>> -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>>> -	if (IS_ERR(rockchip->vpcie0v9)) {
>>> -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>> -			return PTR_ERR(rockchip->vpcie0v9);
>>> -		dev_info(dev, "no vpcie0v9 regulator found\n");
>>> -	}
>>> +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>> +	if (IS_ERR(rockchip->vpcie0v9))
>>> +		return PTR_ERR(rockchip->vpcie0v9);
>>>   
>>>   	return 0;
>>>   }
>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>>   		}
>>>   	}
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie1v8)) {
>>> -		err = regulator_enable(rockchip->vpcie1v8);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> -			goto err_disable_3v3;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie1v8);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> +		goto err_disable_3v3;
>>>   	}
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			goto err_disable_1v8;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		goto err_disable_1v8;
>>>   	}
>>>   
>>>   	return 0;
>>>   
>>>   err_disable_1v8:
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>>   err_disable_3v3:
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>>   
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   
>>>   	return ret;
>>>   }
>>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>   	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>   	int err;
>>>   
>>> -	if (!IS_ERR(rockchip->vpcie0v9)) {
>>> -		err = regulator_enable(rockchip->vpcie0v9);
>>> -		if (err) {
>>> -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> -			return err;
>>> -		}
>>> +	err = regulator_enable(rockchip->vpcie0v9);
>>> +	if (err) {
>>> +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> +		return err;
>>>   	}
>>>   
>>>   	err = rockchip_pcie_enable_clocks(rockchip);
>>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>>   err_pcie_resume:
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   err_disable_0v9:
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   	return err;
>>>   }
>>>   
>>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>   		regulator_disable(rockchip->vpcie12v);
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   err_set_vpcie:
>>>   	rockchip_pcie_disable_clocks(rockchip);
>>>   	return err;
>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>>   		regulator_disable(rockchip->vpcie12v);
>>>   	if (!IS_ERR(rockchip->vpcie3v3))
>>>   		regulator_disable(rockchip->vpcie3v3);
>>> -	if (!IS_ERR(rockchip->vpcie1v8))
>>> -		regulator_disable(rockchip->vpcie1v8);
>>> -	if (!IS_ERR(rockchip->vpcie0v9))
>>> -		regulator_disable(rockchip->vpcie0v9);
>>> +	regulator_disable(rockchip->vpcie1v8);
>>> +	regulator_disable(rockchip->vpcie0v9);
>>>   
>>>   	return 0;
>>>   }
>>> -- 
>>> 2.17.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午8:47, Robin Murphy wrote:
> On 2020-11-07 11:36, Qu Wenruo wrote:
>>
>>
>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>> not explicitly described.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>   drivers/pci/controller/pcie-rockchip-host.c | 69
>>>> ++++++++-------------
>>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>>
>>> Applied to pci/rockchip, thanks.
>>
>> Sorry, this commit is cause regression for RK3399 boards unable to
>> detect the controller anymore.
>>
>> The 1v8 (and 0v9) is causing -517 and reject the controller
>> initialization.
> 
> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
> but you're missing the relevant driver - that's an issue with your
> config/initrd. Being optional should only change the behaviour if the
> supply is totally absent (i.e. you get -ENODEV instead of a dummy
> regulator), so I don't see that it would make any difference in this
> situation anyway :/
> 
>> I'm not a PCI guy, but a quick google search shows these two voltages
>> are not related to PCIE core functionality, especially considering the
>> controller used in RK3399 are mostly to provide NVME support.
> 
> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
> datasheet describe as "Supply voltage for PCIE". Having power is kind of
> important for the I/O circuits on all the signal pins to work.
> 
> Now it's almost certainly true that these supplies technically belong to
> the phy rather than the controller, but it's a bit late to change the
> bindings for the sake of semantics.
> 
>> This bug makes all RK3399 users who put root fs into NVME driver unable
>> to boot the device.
>>
>> I really hope some one could test the patch before affecting the end
>> users or at least try to understand how most users would use the PCIE
>> interface for.
> 
> I *am* that end user in this case - I use an M.2 NVME on my board, which
> prompted me to take a look at the regulator handling here in the first
> place, to see if it might be possible to shut up the annoying message
> about a 12V supply that is entirely irrelevant to a board without a
> full-size PCIe slot. I use a mainline-based distro, so I've been running
> this change for nearly a year since it landed in v5.5, and I'm sure many
> others have too. I've not heard of any other complaints in that time...

Sorry for the wrong wording, thank you for your contribution first.

It really makes RK3399 the primary testing bed for 64K page size, and
save me a lot of time.

I'm wondering how the -EPROBE_DEFER happens.
I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
The config works for rc4, unless something big changed in rc2, it
shouldn't change much, right?

The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
special driver.
Or does it?

Thanks,
Qu

> 
> Robin.
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> Lorenzo
>>>
>>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
>>>> b/drivers/pci/controller/pcie-rockchip-host.c
>>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct
>>>> rockchip_pcie *rockchip)
>>>>           dev_info(dev, "no vpcie3v3 regulator found\n");
>>>>       }
>>>>   -    rockchip->vpcie1v8 = devm_regulator_get_optional(dev,
>>>> "vpcie1v8");
>>>> -    if (IS_ERR(rockchip->vpcie1v8)) {
>>>> -        if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>>> -            return PTR_ERR(rockchip->vpcie1v8);
>>>> -        dev_info(dev, "no vpcie1v8 regulator found\n");
>>>> -    }
>>>> +    rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>>> +    if (IS_ERR(rockchip->vpcie1v8))
>>>> +        return PTR_ERR(rockchip->vpcie1v8);
>>>>   -    rockchip->vpcie0v9 = devm_regulator_get_optional(dev,
>>>> "vpcie0v9");
>>>> -    if (IS_ERR(rockchip->vpcie0v9)) {
>>>> -        if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>>> -            return PTR_ERR(rockchip->vpcie0v9);
>>>> -        dev_info(dev, "no vpcie0v9 regulator found\n");
>>>> -    }
>>>> +    rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>>> +    if (IS_ERR(rockchip->vpcie0v9))
>>>> +        return PTR_ERR(rockchip->vpcie0v9);
>>>>         return 0;
>>>>   }
>>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct
>>>> rockchip_pcie *rockchip)
>>>>           }
>>>>       }
>>>>   -    if (!IS_ERR(rockchip->vpcie1v8)) {
>>>> -        err = regulator_enable(rockchip->vpcie1v8);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>> -            goto err_disable_3v3;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie1v8);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>> +        goto err_disable_3v3;
>>>>       }
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> -            goto err_disable_1v8;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> +        goto err_disable_1v8;
>>>>       }
>>>>         return 0;
>>>>     err_disable_1v8:
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>   err_disable_3v3:
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> @@ -897,8 +886,7 @@ static int __maybe_unused
>>>> rockchip_pcie_suspend_noirq(struct device *dev)
>>>>         rockchip_pcie_disable_clocks(rockchip);
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>         return ret;
>>>>   }
>>>> @@ -908,12 +896,10 @@ static int __maybe_unused
>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>       struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>>       int err;
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> -            return err;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> +        return err;
>>>>       }
>>>>         err = rockchip_pcie_enable_clocks(rockchip);
>>>> @@ -939,8 +925,7 @@ static int __maybe_unused
>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>   err_pcie_resume:
>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>   err_disable_0v9:
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>       return err;
>>>>   }
>>>>   @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct
>>>> platform_device *pdev)
>>>>           regulator_disable(rockchip->vpcie12v);
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>   err_set_vpcie:
>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>       return err;
>>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct
>>>> platform_device *pdev)
>>>>           regulator_disable(rockchip->vpcie12v);
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>         return 0;
>>>>   }
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午8:47, Robin Murphy wrote:
> On 2020-11-07 11:36, Qu Wenruo wrote:
>>
>>
>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>> not explicitly described.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>   drivers/pci/controller/pcie-rockchip-host.c | 69
>>>> ++++++++-------------
>>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>>
>>> Applied to pci/rockchip, thanks.
>>
>> Sorry, this commit is cause regression for RK3399 boards unable to
>> detect the controller anymore.
>>
>> The 1v8 (and 0v9) is causing -517 and reject the controller
>> initialization.
> 
> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
> but you're missing the relevant driver - that's an issue with your
> config/initrd. Being optional should only change the behaviour if the
> supply is totally absent (i.e. you get -ENODEV instead of a dummy
> regulator), so I don't see that it would make any difference in this
> situation anyway :/
> 
>> I'm not a PCI guy, but a quick google search shows these two voltages
>> are not related to PCIE core functionality, especially considering the
>> controller used in RK3399 are mostly to provide NVME support.
> 
> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
> datasheet describe as "Supply voltage for PCIE". Having power is kind of
> important for the I/O circuits on all the signal pins to work.
> 
> Now it's almost certainly true that these supplies technically belong to
> the phy rather than the controller, but it's a bit late to change the
> bindings for the sake of semantics.
> 
>> This bug makes all RK3399 users who put root fs into NVME driver unable
>> to boot the device.
>>
>> I really hope some one could test the patch before affecting the end
>> users or at least try to understand how most users would use the PCIE
>> interface for.
> 
> I *am* that end user in this case - I use an M.2 NVME on my board, which
> prompted me to take a look at the regulator handling here in the first
> place, to see if it might be possible to shut up the annoying message
> about a 12V supply that is entirely irrelevant to a board without a
> full-size PCIe slot. I use a mainline-based distro, so I've been running
> this change for nearly a year since it landed in v5.5, and I'm sure many
> others have too. I've not heard of any other complaints in that time...

Sorry for the wrong wording, thank you for your contribution first.

It really makes RK3399 the primary testing bed for 64K page size, and
save me a lot of time.

I'm wondering how the -EPROBE_DEFER happens.
I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
The config works for rc4, unless something big changed in rc2, it
shouldn't change much, right?

The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
special driver.
Or does it?

Thanks,
Qu

> 
> Robin.
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> Lorenzo
>>>
>>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
>>>> b/drivers/pci/controller/pcie-rockchip-host.c
>>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct
>>>> rockchip_pcie *rockchip)
>>>>           dev_info(dev, "no vpcie3v3 regulator found\n");
>>>>       }
>>>>   -    rockchip->vpcie1v8 = devm_regulator_get_optional(dev,
>>>> "vpcie1v8");
>>>> -    if (IS_ERR(rockchip->vpcie1v8)) {
>>>> -        if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>>> -            return PTR_ERR(rockchip->vpcie1v8);
>>>> -        dev_info(dev, "no vpcie1v8 regulator found\n");
>>>> -    }
>>>> +    rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>>> +    if (IS_ERR(rockchip->vpcie1v8))
>>>> +        return PTR_ERR(rockchip->vpcie1v8);
>>>>   -    rockchip->vpcie0v9 = devm_regulator_get_optional(dev,
>>>> "vpcie0v9");
>>>> -    if (IS_ERR(rockchip->vpcie0v9)) {
>>>> -        if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>>> -            return PTR_ERR(rockchip->vpcie0v9);
>>>> -        dev_info(dev, "no vpcie0v9 regulator found\n");
>>>> -    }
>>>> +    rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>>> +    if (IS_ERR(rockchip->vpcie0v9))
>>>> +        return PTR_ERR(rockchip->vpcie0v9);
>>>>         return 0;
>>>>   }
>>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct
>>>> rockchip_pcie *rockchip)
>>>>           }
>>>>       }
>>>>   -    if (!IS_ERR(rockchip->vpcie1v8)) {
>>>> -        err = regulator_enable(rockchip->vpcie1v8);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>> -            goto err_disable_3v3;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie1v8);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>> +        goto err_disable_3v3;
>>>>       }
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> -            goto err_disable_1v8;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> +        goto err_disable_1v8;
>>>>       }
>>>>         return 0;
>>>>     err_disable_1v8:
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>   err_disable_3v3:
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> @@ -897,8 +886,7 @@ static int __maybe_unused
>>>> rockchip_pcie_suspend_noirq(struct device *dev)
>>>>         rockchip_pcie_disable_clocks(rockchip);
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>         return ret;
>>>>   }
>>>> @@ -908,12 +896,10 @@ static int __maybe_unused
>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>       struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>>       int err;
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> -            return err;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> +        return err;
>>>>       }
>>>>         err = rockchip_pcie_enable_clocks(rockchip);
>>>> @@ -939,8 +925,7 @@ static int __maybe_unused
>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>   err_pcie_resume:
>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>   err_disable_0v9:
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>       return err;
>>>>   }
>>>>   @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct
>>>> platform_device *pdev)
>>>>           regulator_disable(rockchip->vpcie12v);
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>   err_set_vpcie:
>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>       return err;
>>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct
>>>> platform_device *pdev)
>>>>           regulator_disable(rockchip->vpcie12v);
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>         return 0;
>>>>   }
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午8:47, Robin Murphy wrote:
> On 2020-11-07 11:36, Qu Wenruo wrote:
>>
>>
>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>> not explicitly described.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>   drivers/pci/controller/pcie-rockchip-host.c | 69
>>>> ++++++++-------------
>>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>>
>>> Applied to pci/rockchip, thanks.
>>
>> Sorry, this commit is cause regression for RK3399 boards unable to
>> detect the controller anymore.
>>
>> The 1v8 (and 0v9) is causing -517 and reject the controller
>> initialization.
> 
> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
> but you're missing the relevant driver - that's an issue with your
> config/initrd. Being optional should only change the behaviour if the
> supply is totally absent (i.e. you get -ENODEV instead of a dummy
> regulator), so I don't see that it would make any difference in this
> situation anyway :/
> 
>> I'm not a PCI guy, but a quick google search shows these two voltages
>> are not related to PCIE core functionality, especially considering the
>> controller used in RK3399 are mostly to provide NVME support.
> 
> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
> datasheet describe as "Supply voltage for PCIE". Having power is kind of
> important for the I/O circuits on all the signal pins to work.
> 
> Now it's almost certainly true that these supplies technically belong to
> the phy rather than the controller, but it's a bit late to change the
> bindings for the sake of semantics.
> 
>> This bug makes all RK3399 users who put root fs into NVME driver unable
>> to boot the device.
>>
>> I really hope some one could test the patch before affecting the end
>> users or at least try to understand how most users would use the PCIE
>> interface for.
> 
> I *am* that end user in this case - I use an M.2 NVME on my board, which
> prompted me to take a look at the regulator handling here in the first
> place, to see if it might be possible to shut up the annoying message
> about a 12V supply that is entirely irrelevant to a board without a
> full-size PCIe slot. I use a mainline-based distro, so I've been running
> this change for nearly a year since it landed in v5.5, and I'm sure many
> others have too. I've not heard of any other complaints in that time...

Sorry for the wrong wording, thank you for your contribution first.

It really makes RK3399 the primary testing bed for 64K page size, and
save me a lot of time.

I'm wondering how the -EPROBE_DEFER happens.
I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
The config works for rc4, unless something big changed in rc2, it
shouldn't change much, right?

The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
special driver.
Or does it?

Thanks,
Qu

> 
> Robin.
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> Lorenzo
>>>
>>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
>>>> b/drivers/pci/controller/pcie-rockchip-host.c
>>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct
>>>> rockchip_pcie *rockchip)
>>>>           dev_info(dev, "no vpcie3v3 regulator found\n");
>>>>       }
>>>>   -    rockchip->vpcie1v8 = devm_regulator_get_optional(dev,
>>>> "vpcie1v8");
>>>> -    if (IS_ERR(rockchip->vpcie1v8)) {
>>>> -        if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>>> -            return PTR_ERR(rockchip->vpcie1v8);
>>>> -        dev_info(dev, "no vpcie1v8 regulator found\n");
>>>> -    }
>>>> +    rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>>> +    if (IS_ERR(rockchip->vpcie1v8))
>>>> +        return PTR_ERR(rockchip->vpcie1v8);
>>>>   -    rockchip->vpcie0v9 = devm_regulator_get_optional(dev,
>>>> "vpcie0v9");
>>>> -    if (IS_ERR(rockchip->vpcie0v9)) {
>>>> -        if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>>> -            return PTR_ERR(rockchip->vpcie0v9);
>>>> -        dev_info(dev, "no vpcie0v9 regulator found\n");
>>>> -    }
>>>> +    rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>>> +    if (IS_ERR(rockchip->vpcie0v9))
>>>> +        return PTR_ERR(rockchip->vpcie0v9);
>>>>         return 0;
>>>>   }
>>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct
>>>> rockchip_pcie *rockchip)
>>>>           }
>>>>       }
>>>>   -    if (!IS_ERR(rockchip->vpcie1v8)) {
>>>> -        err = regulator_enable(rockchip->vpcie1v8);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>> -            goto err_disable_3v3;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie1v8);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>> +        goto err_disable_3v3;
>>>>       }
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> -            goto err_disable_1v8;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> +        goto err_disable_1v8;
>>>>       }
>>>>         return 0;
>>>>     err_disable_1v8:
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>   err_disable_3v3:
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> @@ -897,8 +886,7 @@ static int __maybe_unused
>>>> rockchip_pcie_suspend_noirq(struct device *dev)
>>>>         rockchip_pcie_disable_clocks(rockchip);
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>         return ret;
>>>>   }
>>>> @@ -908,12 +896,10 @@ static int __maybe_unused
>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>       struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>>       int err;
>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>> -        if (err) {
>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> -            return err;
>>>> -        }
>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>> +    if (err) {
>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>> +        return err;
>>>>       }
>>>>         err = rockchip_pcie_enable_clocks(rockchip);
>>>> @@ -939,8 +925,7 @@ static int __maybe_unused
>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>   err_pcie_resume:
>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>   err_disable_0v9:
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>       return err;
>>>>   }
>>>>   @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct
>>>> platform_device *pdev)
>>>>           regulator_disable(rockchip->vpcie12v);
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>   err_set_vpcie:
>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>       return err;
>>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct
>>>> platform_device *pdev)
>>>>           regulator_disable(rockchip->vpcie12v);
>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>           regulator_disable(rockchip->vpcie3v3);
>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>         return 0;
>>>>   }
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午8:54, Qu Wenruo wrote:
> 
> 
> On 2020/11/7 下午8:47, Robin Murphy wrote:
>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>
>>>
>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>> not explicitly described.
>>>>>
>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>> ---
>>>>>   drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>> ++++++++-------------
>>>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>>>
>>>> Applied to pci/rockchip, thanks.
>>>
>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>> detect the controller anymore.
>>>
>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>> initialization.
>>
>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>> but you're missing the relevant driver - that's an issue with your
>> config/initrd. Being optional should only change the behaviour if the
>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>> regulator), so I don't see that it would make any difference in this
>> situation anyway :/
>>
>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>> are not related to PCIE core functionality, especially considering the
>>> controller used in RK3399 are mostly to provide NVME support.
>>
>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>> datasheet describe as "Supply voltage for PCIE". Having power is kind of
>> important for the I/O circuits on all the signal pins to work.
>>
>> Now it's almost certainly true that these supplies technically belong to
>> the phy rather than the controller, but it's a bit late to change the
>> bindings for the sake of semantics.
>>
>>> This bug makes all RK3399 users who put root fs into NVME driver unable
>>> to boot the device.
>>>
>>> I really hope some one could test the patch before affecting the end
>>> users or at least try to understand how most users would use the PCIE
>>> interface for.
>>
>> I *am* that end user in this case - I use an M.2 NVME on my board, which
>> prompted me to take a look at the regulator handling here in the first
>> place, to see if it might be possible to shut up the annoying message
>> about a 12V supply that is entirely irrelevant to a board without a
>> full-size PCIe slot. I use a mainline-based distro, so I've been running
>> this change for nearly a year since it landed in v5.5, and I'm sure many
>> others have too. I've not heard of any other complaints in that time...
> 
> Sorry for the wrong wording, thank you for your contribution first.
> 
> It really makes RK3399 the primary testing bed for 64K page size, and
> save me a lot of time.
> 
> I'm wondering how the -EPROBE_DEFER happens.
> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
> The config works for rc4, unless something big changed in rc2, it
> shouldn't change much, right?
> 
> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
> special driver.
> Or does it?

Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
are all provided by rk808, while the dmesg shows:

[    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
vin-supply from device tree
[    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
supplying 3300000uV
[    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
vin-supply from device tree
[    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
supplying 0uV
[    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
vin-supply from device tree
[    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
supplying 0uV
[    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
3300000uV
[    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
[    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from device tree
[    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
node /vdd-log failed
[    1.086501] vdd_log: supplied by regulator-dummy
[    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
[    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
[    1.419856] rk808 0-001b: failed to register 12 regulator
[    1.422801] rk808-regulator: probe of rk808-regulator failed with
error -22

So this means something wrong with the rk808, resulting no voltage
provided from rk808 and screwing up the pcie controller?

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>>
>> Robin.
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> Lorenzo
>>>>
>>>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
>>>>> b/drivers/pci/controller/pcie-rockchip-host.c
>>>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct
>>>>> rockchip_pcie *rockchip)
>>>>>           dev_info(dev, "no vpcie3v3 regulator found\n");
>>>>>       }
>>>>>   -    rockchip->vpcie1v8 = devm_regulator_get_optional(dev,
>>>>> "vpcie1v8");
>>>>> -    if (IS_ERR(rockchip->vpcie1v8)) {
>>>>> -        if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>>>> -            return PTR_ERR(rockchip->vpcie1v8);
>>>>> -        dev_info(dev, "no vpcie1v8 regulator found\n");
>>>>> -    }
>>>>> +    rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>>>> +    if (IS_ERR(rockchip->vpcie1v8))
>>>>> +        return PTR_ERR(rockchip->vpcie1v8);
>>>>>   -    rockchip->vpcie0v9 = devm_regulator_get_optional(dev,
>>>>> "vpcie0v9");
>>>>> -    if (IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>>>> -            return PTR_ERR(rockchip->vpcie0v9);
>>>>> -        dev_info(dev, "no vpcie0v9 regulator found\n");
>>>>> -    }
>>>>> +    rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>>>> +    if (IS_ERR(rockchip->vpcie0v9))
>>>>> +        return PTR_ERR(rockchip->vpcie0v9);
>>>>>         return 0;
>>>>>   }
>>>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct
>>>>> rockchip_pcie *rockchip)
>>>>>           }
>>>>>       }
>>>>>   -    if (!IS_ERR(rockchip->vpcie1v8)) {
>>>>> -        err = regulator_enable(rockchip->vpcie1v8);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>>> -            goto err_disable_3v3;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie1v8);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>>> +        goto err_disable_3v3;
>>>>>       }
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> -            goto err_disable_1v8;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> +        goto err_disable_1v8;
>>>>>       }
>>>>>         return 0;
>>>>>     err_disable_1v8:
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>>   err_disable_3v3:
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> @@ -897,8 +886,7 @@ static int __maybe_unused
>>>>> rockchip_pcie_suspend_noirq(struct device *dev)
>>>>>         rockchip_pcie_disable_clocks(rockchip);
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>         return ret;
>>>>>   }
>>>>> @@ -908,12 +896,10 @@ static int __maybe_unused
>>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>>       struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>>>       int err;
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> -            return err;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> +        return err;
>>>>>       }
>>>>>         err = rockchip_pcie_enable_clocks(rockchip);
>>>>> @@ -939,8 +925,7 @@ static int __maybe_unused
>>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>>   err_pcie_resume:
>>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>>   err_disable_0v9:
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>       return err;
>>>>>   }
>>>>>   @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct
>>>>> platform_device *pdev)
>>>>>           regulator_disable(rockchip->vpcie12v);
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>   err_set_vpcie:
>>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>>       return err;
>>>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct
>>>>> platform_device *pdev)
>>>>>           regulator_disable(rockchip->vpcie12v);
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>         return 0;
>>>>>   }
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午8:54, Qu Wenruo wrote:
> 
> 
> On 2020/11/7 下午8:47, Robin Murphy wrote:
>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>
>>>
>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>> not explicitly described.
>>>>>
>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>> ---
>>>>>   drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>> ++++++++-------------
>>>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>>>
>>>> Applied to pci/rockchip, thanks.
>>>
>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>> detect the controller anymore.
>>>
>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>> initialization.
>>
>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>> but you're missing the relevant driver - that's an issue with your
>> config/initrd. Being optional should only change the behaviour if the
>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>> regulator), so I don't see that it would make any difference in this
>> situation anyway :/
>>
>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>> are not related to PCIE core functionality, especially considering the
>>> controller used in RK3399 are mostly to provide NVME support.
>>
>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>> datasheet describe as "Supply voltage for PCIE". Having power is kind of
>> important for the I/O circuits on all the signal pins to work.
>>
>> Now it's almost certainly true that these supplies technically belong to
>> the phy rather than the controller, but it's a bit late to change the
>> bindings for the sake of semantics.
>>
>>> This bug makes all RK3399 users who put root fs into NVME driver unable
>>> to boot the device.
>>>
>>> I really hope some one could test the patch before affecting the end
>>> users or at least try to understand how most users would use the PCIE
>>> interface for.
>>
>> I *am* that end user in this case - I use an M.2 NVME on my board, which
>> prompted me to take a look at the regulator handling here in the first
>> place, to see if it might be possible to shut up the annoying message
>> about a 12V supply that is entirely irrelevant to a board without a
>> full-size PCIe slot. I use a mainline-based distro, so I've been running
>> this change for nearly a year since it landed in v5.5, and I'm sure many
>> others have too. I've not heard of any other complaints in that time...
> 
> Sorry for the wrong wording, thank you for your contribution first.
> 
> It really makes RK3399 the primary testing bed for 64K page size, and
> save me a lot of time.
> 
> I'm wondering how the -EPROBE_DEFER happens.
> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
> The config works for rc4, unless something big changed in rc2, it
> shouldn't change much, right?
> 
> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
> special driver.
> Or does it?

Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
are all provided by rk808, while the dmesg shows:

[    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
vin-supply from device tree
[    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
supplying 3300000uV
[    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
vin-supply from device tree
[    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
supplying 0uV
[    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
vin-supply from device tree
[    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
supplying 0uV
[    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
3300000uV
[    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
[    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from device tree
[    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
node /vdd-log failed
[    1.086501] vdd_log: supplied by regulator-dummy
[    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
[    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
[    1.419856] rk808 0-001b: failed to register 12 regulator
[    1.422801] rk808-regulator: probe of rk808-regulator failed with
error -22

So this means something wrong with the rk808, resulting no voltage
provided from rk808 and screwing up the pcie controller?

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>>
>> Robin.
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> Lorenzo
>>>>
>>>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
>>>>> b/drivers/pci/controller/pcie-rockchip-host.c
>>>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct
>>>>> rockchip_pcie *rockchip)
>>>>>           dev_info(dev, "no vpcie3v3 regulator found\n");
>>>>>       }
>>>>>   -    rockchip->vpcie1v8 = devm_regulator_get_optional(dev,
>>>>> "vpcie1v8");
>>>>> -    if (IS_ERR(rockchip->vpcie1v8)) {
>>>>> -        if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>>>> -            return PTR_ERR(rockchip->vpcie1v8);
>>>>> -        dev_info(dev, "no vpcie1v8 regulator found\n");
>>>>> -    }
>>>>> +    rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>>>> +    if (IS_ERR(rockchip->vpcie1v8))
>>>>> +        return PTR_ERR(rockchip->vpcie1v8);
>>>>>   -    rockchip->vpcie0v9 = devm_regulator_get_optional(dev,
>>>>> "vpcie0v9");
>>>>> -    if (IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>>>> -            return PTR_ERR(rockchip->vpcie0v9);
>>>>> -        dev_info(dev, "no vpcie0v9 regulator found\n");
>>>>> -    }
>>>>> +    rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>>>> +    if (IS_ERR(rockchip->vpcie0v9))
>>>>> +        return PTR_ERR(rockchip->vpcie0v9);
>>>>>         return 0;
>>>>>   }
>>>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct
>>>>> rockchip_pcie *rockchip)
>>>>>           }
>>>>>       }
>>>>>   -    if (!IS_ERR(rockchip->vpcie1v8)) {
>>>>> -        err = regulator_enable(rockchip->vpcie1v8);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>>> -            goto err_disable_3v3;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie1v8);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>>> +        goto err_disable_3v3;
>>>>>       }
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> -            goto err_disable_1v8;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> +        goto err_disable_1v8;
>>>>>       }
>>>>>         return 0;
>>>>>     err_disable_1v8:
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>>   err_disable_3v3:
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> @@ -897,8 +886,7 @@ static int __maybe_unused
>>>>> rockchip_pcie_suspend_noirq(struct device *dev)
>>>>>         rockchip_pcie_disable_clocks(rockchip);
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>         return ret;
>>>>>   }
>>>>> @@ -908,12 +896,10 @@ static int __maybe_unused
>>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>>       struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>>>       int err;
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> -            return err;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> +        return err;
>>>>>       }
>>>>>         err = rockchip_pcie_enable_clocks(rockchip);
>>>>> @@ -939,8 +925,7 @@ static int __maybe_unused
>>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>>   err_pcie_resume:
>>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>>   err_disable_0v9:
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>       return err;
>>>>>   }
>>>>>   @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct
>>>>> platform_device *pdev)
>>>>>           regulator_disable(rockchip->vpcie12v);
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>   err_set_vpcie:
>>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>>       return err;
>>>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct
>>>>> platform_device *pdev)
>>>>>           regulator_disable(rockchip->vpcie12v);
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>         return 0;
>>>>>   }
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/7 下午8:54, Qu Wenruo wrote:
> 
> 
> On 2020/11/7 下午8:47, Robin Murphy wrote:
>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>
>>>
>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>> not explicitly described.
>>>>>
>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>> ---
>>>>>   drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>> ++++++++-------------
>>>>>   1 file changed, 25 insertions(+), 44 deletions(-)
>>>>
>>>> Applied to pci/rockchip, thanks.
>>>
>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>> detect the controller anymore.
>>>
>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>> initialization.
>>
>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>> but you're missing the relevant driver - that's an issue with your
>> config/initrd. Being optional should only change the behaviour if the
>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>> regulator), so I don't see that it would make any difference in this
>> situation anyway :/
>>
>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>> are not related to PCIE core functionality, especially considering the
>>> controller used in RK3399 are mostly to provide NVME support.
>>
>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>> datasheet describe as "Supply voltage for PCIE". Having power is kind of
>> important for the I/O circuits on all the signal pins to work.
>>
>> Now it's almost certainly true that these supplies technically belong to
>> the phy rather than the controller, but it's a bit late to change the
>> bindings for the sake of semantics.
>>
>>> This bug makes all RK3399 users who put root fs into NVME driver unable
>>> to boot the device.
>>>
>>> I really hope some one could test the patch before affecting the end
>>> users or at least try to understand how most users would use the PCIE
>>> interface for.
>>
>> I *am* that end user in this case - I use an M.2 NVME on my board, which
>> prompted me to take a look at the regulator handling here in the first
>> place, to see if it might be possible to shut up the annoying message
>> about a 12V supply that is entirely irrelevant to a board without a
>> full-size PCIe slot. I use a mainline-based distro, so I've been running
>> this change for nearly a year since it landed in v5.5, and I'm sure many
>> others have too. I've not heard of any other complaints in that time...
> 
> Sorry for the wrong wording, thank you for your contribution first.
> 
> It really makes RK3399 the primary testing bed for 64K page size, and
> save me a lot of time.
> 
> I'm wondering how the -EPROBE_DEFER happens.
> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
> The config works for rc4, unless something big changed in rc2, it
> shouldn't change much, right?
> 
> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
> special driver.
> Or does it?

Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
are all provided by rk808, while the dmesg shows:

[    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
vin-supply from device tree
[    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
supplying 3300000uV
[    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
vin-supply from device tree
[    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
supplying 0uV
[    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
vin-supply from device tree
[    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
supplying 0uV
[    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
3300000uV
[    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
[    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from device tree
[    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
node /vdd-log failed
[    1.086501] vdd_log: supplied by regulator-dummy
[    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
[    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
[    1.419856] rk808 0-001b: failed to register 12 regulator
[    1.422801] rk808-regulator: probe of rk808-regulator failed with
error -22

So this means something wrong with the rk808, resulting no voltage
provided from rk808 and screwing up the pcie controller?

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>>
>> Robin.
>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>> Lorenzo
>>>>
>>>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
>>>>> b/drivers/pci/controller/pcie-rockchip-host.c
>>>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct
>>>>> rockchip_pcie *rockchip)
>>>>>           dev_info(dev, "no vpcie3v3 regulator found\n");
>>>>>       }
>>>>>   -    rockchip->vpcie1v8 = devm_regulator_get_optional(dev,
>>>>> "vpcie1v8");
>>>>> -    if (IS_ERR(rockchip->vpcie1v8)) {
>>>>> -        if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>>>> -            return PTR_ERR(rockchip->vpcie1v8);
>>>>> -        dev_info(dev, "no vpcie1v8 regulator found\n");
>>>>> -    }
>>>>> +    rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>>>> +    if (IS_ERR(rockchip->vpcie1v8))
>>>>> +        return PTR_ERR(rockchip->vpcie1v8);
>>>>>   -    rockchip->vpcie0v9 = devm_regulator_get_optional(dev,
>>>>> "vpcie0v9");
>>>>> -    if (IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>>>> -            return PTR_ERR(rockchip->vpcie0v9);
>>>>> -        dev_info(dev, "no vpcie0v9 regulator found\n");
>>>>> -    }
>>>>> +    rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>>>> +    if (IS_ERR(rockchip->vpcie0v9))
>>>>> +        return PTR_ERR(rockchip->vpcie0v9);
>>>>>         return 0;
>>>>>   }
>>>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct
>>>>> rockchip_pcie *rockchip)
>>>>>           }
>>>>>       }
>>>>>   -    if (!IS_ERR(rockchip->vpcie1v8)) {
>>>>> -        err = regulator_enable(rockchip->vpcie1v8);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>>> -            goto err_disable_3v3;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie1v8);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>>>> +        goto err_disable_3v3;
>>>>>       }
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> -            goto err_disable_1v8;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> +        goto err_disable_1v8;
>>>>>       }
>>>>>         return 0;
>>>>>     err_disable_1v8:
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>>   err_disable_3v3:
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> @@ -897,8 +886,7 @@ static int __maybe_unused
>>>>> rockchip_pcie_suspend_noirq(struct device *dev)
>>>>>         rockchip_pcie_disable_clocks(rockchip);
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>         return ret;
>>>>>   }
>>>>> @@ -908,12 +896,10 @@ static int __maybe_unused
>>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>>       struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>>>>       int err;
>>>>>   -    if (!IS_ERR(rockchip->vpcie0v9)) {
>>>>> -        err = regulator_enable(rockchip->vpcie0v9);
>>>>> -        if (err) {
>>>>> -            dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> -            return err;
>>>>> -        }
>>>>> +    err = regulator_enable(rockchip->vpcie0v9);
>>>>> +    if (err) {
>>>>> +        dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>>>> +        return err;
>>>>>       }
>>>>>         err = rockchip_pcie_enable_clocks(rockchip);
>>>>> @@ -939,8 +925,7 @@ static int __maybe_unused
>>>>> rockchip_pcie_resume_noirq(struct device *dev)
>>>>>   err_pcie_resume:
>>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>>   err_disable_0v9:
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>       return err;
>>>>>   }
>>>>>   @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct
>>>>> platform_device *pdev)
>>>>>           regulator_disable(rockchip->vpcie12v);
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>   err_set_vpcie:
>>>>>       rockchip_pcie_disable_clocks(rockchip);
>>>>>       return err;
>>>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct
>>>>> platform_device *pdev)
>>>>>           regulator_disable(rockchip->vpcie12v);
>>>>>       if (!IS_ERR(rockchip->vpcie3v3))
>>>>>           regulator_disable(rockchip->vpcie3v3);
>>>>> -    if (!IS_ERR(rockchip->vpcie1v8))
>>>>> -        regulator_disable(rockchip->vpcie1v8);
>>>>> -    if (!IS_ERR(rockchip->vpcie0v9))
>>>>> -        regulator_disable(rockchip->vpcie0v9);
>>>>> +    regulator_disable(rockchip->vpcie1v8);
>>>>> +    regulator_disable(rockchip->vpcie0v9);
>>>>>         return 0;
>>>>>   }
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

On 2020-11-07 13:30, Qu Wenruo wrote:
> 
> 
> On 2020/11/7 下午8:54, Qu Wenruo wrote:
>>
>>
>> On 2020/11/7 下午8:47, Robin Murphy wrote:
>>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>>> not explicitly described.
>>>>>>
>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>>> ---
>>>>>>    drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>>> ++++++++-------------
>>>>>>    1 file changed, 25 insertions(+), 44 deletions(-)
>>>>>
>>>>> Applied to pci/rockchip, thanks.
>>>>
>>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>>> detect the controller anymore.
>>>>
>>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>>> initialization.
>>>
>>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>>> but you're missing the relevant driver - that's an issue with your
>>> config/initrd. Being optional should only change the behaviour if the
>>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>>> regulator), so I don't see that it would make any difference in this
>>> situation anyway :/
>>>
>>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>>> are not related to PCIE core functionality, especially considering the
>>>> controller used in RK3399 are mostly to provide NVME support.
>>>
>>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>>> datasheet describe as "Supply voltage for PCIE". Having power is kind of
>>> important for the I/O circuits on all the signal pins to work.
>>>
>>> Now it's almost certainly true that these supplies technically belong to
>>> the phy rather than the controller, but it's a bit late to change the
>>> bindings for the sake of semantics.
>>>
>>>> This bug makes all RK3399 users who put root fs into NVME driver unable
>>>> to boot the device.
>>>>
>>>> I really hope some one could test the patch before affecting the end
>>>> users or at least try to understand how most users would use the PCIE
>>>> interface for.
>>>
>>> I *am* that end user in this case - I use an M.2 NVME on my board, which
>>> prompted me to take a look at the regulator handling here in the first
>>> place, to see if it might be possible to shut up the annoying message
>>> about a 12V supply that is entirely irrelevant to a board without a
>>> full-size PCIe slot. I use a mainline-based distro, so I've been running
>>> this change for nearly a year since it landed in v5.5, and I'm sure many
>>> others have too. I've not heard of any other complaints in that time...
>>
>> Sorry for the wrong wording, thank you for your contribution first.
>>
>> It really makes RK3399 the primary testing bed for 64K page size, and
>> save me a lot of time.
>>
>> I'm wondering how the -EPROBE_DEFER happens.
>> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
>> The config works for rc4, unless something big changed in rc2, it
>> shouldn't change much, right?
>>
>> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
>> special driver.
>> Or does it?
> 
> Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
> are all provided by rk808, while the dmesg shows:
> 
> [    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
> vin-supply from device tree
> [    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
> supplying 3300000uV
> [    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
> vin-supply from device tree
> [    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
> supplying 0uV
> [    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
> vin-supply from device tree
> [    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
> supplying 0uV
> [    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
> 3300000uV
> [    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
> [    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from device tree
> [    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
> node /vdd-log failed
> [    1.086501] vdd_log: supplied by regulator-dummy
> [    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
> [    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
> [    1.419856] rk808 0-001b: failed to register 12 regulator
> [    1.422801] rk808-regulator: probe of rk808-regulator failed with
> error -22
> 
> So this means something wrong with the rk808, resulting no voltage
> provided from rk808 and screwing up the pcie controller?

Frankly PCIe is the least of your worries there - if the system PMIC is 
failing to probe then pretty much everything will be degraded: cpufreq 
won't work, some SD card modes won't work, on some boards entire 
peripherals like ethernet might not work if they're wired up to use the 
opposite I/O domain voltage setting from the SoC's power-on default.

Focusing on PCIe getting probe-deferred seems a bit like complaining 
that the carpets on the Titanic are wet ;)

Robin.

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

On 2020-11-07 13:30, Qu Wenruo wrote:
> 
> 
> On 2020/11/7 下午8:54, Qu Wenruo wrote:
>>
>>
>> On 2020/11/7 下午8:47, Robin Murphy wrote:
>>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>>> not explicitly described.
>>>>>>
>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>>> ---
>>>>>>    drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>>> ++++++++-------------
>>>>>>    1 file changed, 25 insertions(+), 44 deletions(-)
>>>>>
>>>>> Applied to pci/rockchip, thanks.
>>>>
>>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>>> detect the controller anymore.
>>>>
>>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>>> initialization.
>>>
>>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>>> but you're missing the relevant driver - that's an issue with your
>>> config/initrd. Being optional should only change the behaviour if the
>>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>>> regulator), so I don't see that it would make any difference in this
>>> situation anyway :/
>>>
>>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>>> are not related to PCIE core functionality, especially considering the
>>>> controller used in RK3399 are mostly to provide NVME support.
>>>
>>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>>> datasheet describe as "Supply voltage for PCIE". Having power is kind of
>>> important for the I/O circuits on all the signal pins to work.
>>>
>>> Now it's almost certainly true that these supplies technically belong to
>>> the phy rather than the controller, but it's a bit late to change the
>>> bindings for the sake of semantics.
>>>
>>>> This bug makes all RK3399 users who put root fs into NVME driver unable
>>>> to boot the device.
>>>>
>>>> I really hope some one could test the patch before affecting the end
>>>> users or at least try to understand how most users would use the PCIE
>>>> interface for.
>>>
>>> I *am* that end user in this case - I use an M.2 NVME on my board, which
>>> prompted me to take a look at the regulator handling here in the first
>>> place, to see if it might be possible to shut up the annoying message
>>> about a 12V supply that is entirely irrelevant to a board without a
>>> full-size PCIe slot. I use a mainline-based distro, so I've been running
>>> this change for nearly a year since it landed in v5.5, and I'm sure many
>>> others have too. I've not heard of any other complaints in that time...
>>
>> Sorry for the wrong wording, thank you for your contribution first.
>>
>> It really makes RK3399 the primary testing bed for 64K page size, and
>> save me a lot of time.
>>
>> I'm wondering how the -EPROBE_DEFER happens.
>> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
>> The config works for rc4, unless something big changed in rc2, it
>> shouldn't change much, right?
>>
>> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
>> special driver.
>> Or does it?
> 
> Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
> are all provided by rk808, while the dmesg shows:
> 
> [    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
> vin-supply from device tree
> [    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
> supplying 3300000uV
> [    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
> vin-supply from device tree
> [    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
> supplying 0uV
> [    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
> vin-supply from device tree
> [    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
> supplying 0uV
> [    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
> 3300000uV
> [    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
> [    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from device tree
> [    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
> node /vdd-log failed
> [    1.086501] vdd_log: supplied by regulator-dummy
> [    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
> [    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
> [    1.419856] rk808 0-001b: failed to register 12 regulator
> [    1.422801] rk808-regulator: probe of rk808-regulator failed with
> error -22
> 
> So this means something wrong with the rk808, resulting no voltage
> provided from rk808 and screwing up the pcie controller?

Frankly PCIe is the least of your worries there - if the system PMIC is 
failing to probe then pretty much everything will be degraded: cpufreq 
won't work, some SD card modes won't work, on some boards entire 
peripherals like ethernet might not work if they're wired up to use the 
opposite I/O domain voltage setting from the SoC's power-on default.

Focusing on PCIe getting probe-deferred seems a bit like complaining 
that the carpets on the Titanic are wet ;)

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy]

On 2020-11-07 13:30, Qu Wenruo wrote:
> 
> 
> On 2020/11/7 下午8:54, Qu Wenruo wrote:
>>
>>
>> On 2020/11/7 下午8:47, Robin Murphy wrote:
>>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>>> not explicitly described.
>>>>>>
>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>>> ---
>>>>>>    drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>>> ++++++++-------------
>>>>>>    1 file changed, 25 insertions(+), 44 deletions(-)
>>>>>
>>>>> Applied to pci/rockchip, thanks.
>>>>
>>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>>> detect the controller anymore.
>>>>
>>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>>> initialization.
>>>
>>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>>> but you're missing the relevant driver - that's an issue with your
>>> config/initrd. Being optional should only change the behaviour if the
>>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>>> regulator), so I don't see that it would make any difference in this
>>> situation anyway :/
>>>
>>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>>> are not related to PCIE core functionality, especially considering the
>>>> controller used in RK3399 are mostly to provide NVME support.
>>>
>>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>>> datasheet describe as "Supply voltage for PCIE". Having power is kind of
>>> important for the I/O circuits on all the signal pins to work.
>>>
>>> Now it's almost certainly true that these supplies technically belong to
>>> the phy rather than the controller, but it's a bit late to change the
>>> bindings for the sake of semantics.
>>>
>>>> This bug makes all RK3399 users who put root fs into NVME driver unable
>>>> to boot the device.
>>>>
>>>> I really hope some one could test the patch before affecting the end
>>>> users or at least try to understand how most users would use the PCIE
>>>> interface for.
>>>
>>> I *am* that end user in this case - I use an M.2 NVME on my board, which
>>> prompted me to take a look at the regulator handling here in the first
>>> place, to see if it might be possible to shut up the annoying message
>>> about a 12V supply that is entirely irrelevant to a board without a
>>> full-size PCIe slot. I use a mainline-based distro, so I've been running
>>> this change for nearly a year since it landed in v5.5, and I'm sure many
>>> others have too. I've not heard of any other complaints in that time...
>>
>> Sorry for the wrong wording, thank you for your contribution first.
>>
>> It really makes RK3399 the primary testing bed for 64K page size, and
>> save me a lot of time.
>>
>> I'm wondering how the -EPROBE_DEFER happens.
>> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
>> The config works for rc4, unless something big changed in rc2, it
>> shouldn't change much, right?
>>
>> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
>> special driver.
>> Or does it?
> 
> Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
> are all provided by rk808, while the dmesg shows:
> 
> [    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
> vin-supply from device tree
> [    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
> supplying 3300000uV
> [    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
> vin-supply from device tree
> [    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
> supplying 0uV
> [    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
> vin-supply from device tree
> [    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
> supplying 0uV
> [    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
> 3300000uV
> [    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
> [    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from device tree
> [    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
> node /vdd-log failed
> [    1.086501] vdd_log: supplied by regulator-dummy
> [    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
> [    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
> [    1.419856] rk808 0-001b: failed to register 12 regulator
> [    1.422801] rk808-regulator: probe of rk808-regulator failed with
> error -22
> 
> So this means something wrong with the rk808, resulting no voltage
> provided from rk808 and screwing up the pcie controller?

Frankly PCIe is the least of your worries there - if the system PMIC is 
failing to probe then pretty much everything will be degraded: cpufreq 
won't work, some SD card modes won't work, on some boards entire 
peripherals like ethernet might not work if they're wired up to use the 
opposite I/O domain voltage setting from the SoC's power-on default.

Focusing on PCIe getting probe-deferred seems a bit like complaining 
that the carpets on the Titanic are wet ;)

Robin.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/9 下午8:00, Robin Murphy wrote:
> On 2020-11-07 13:30, Qu Wenruo wrote:
>>
>>
>> On 2020/11/7 下午8:54, Qu Wenruo wrote:
>>>
>>>
>>> On 2020/11/7 下午8:47, Robin Murphy wrote:
>>>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>>>
>>>>>
>>>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>>>> not explicitly described.
>>>>>>>
>>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>>>> ---
>>>>>>>    drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>>>> ++++++++-------------
>>>>>>>    1 file changed, 25 insertions(+), 44 deletions(-)
>>>>>>
>>>>>> Applied to pci/rockchip, thanks.
>>>>>
>>>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>>>> detect the controller anymore.
>>>>>
>>>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>>>> initialization.
>>>>
>>>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>>>> but you're missing the relevant driver - that's an issue with your
>>>> config/initrd. Being optional should only change the behaviour if the
>>>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>>>> regulator), so I don't see that it would make any difference in this
>>>> situation anyway :/
>>>>
>>>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>>>> are not related to PCIE core functionality, especially considering the
>>>>> controller used in RK3399 are mostly to provide NVME support.
>>>>
>>>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>>>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>>>> datasheet describe as "Supply voltage for PCIE". Having power is
>>>> kind of
>>>> important for the I/O circuits on all the signal pins to work.
>>>>
>>>> Now it's almost certainly true that these supplies technically
>>>> belong to
>>>> the phy rather than the controller, but it's a bit late to change the
>>>> bindings for the sake of semantics.
>>>>
>>>>> This bug makes all RK3399 users who put root fs into NVME driver
>>>>> unable
>>>>> to boot the device.
>>>>>
>>>>> I really hope some one could test the patch before affecting the end
>>>>> users or at least try to understand how most users would use the PCIE
>>>>> interface for.
>>>>
>>>> I *am* that end user in this case - I use an M.2 NVME on my board,
>>>> which
>>>> prompted me to take a look at the regulator handling here in the first
>>>> place, to see if it might be possible to shut up the annoying message
>>>> about a 12V supply that is entirely irrelevant to a board without a
>>>> full-size PCIe slot. I use a mainline-based distro, so I've been
>>>> running
>>>> this change for nearly a year since it landed in v5.5, and I'm sure
>>>> many
>>>> others have too. I've not heard of any other complaints in that time...
>>>
>>> Sorry for the wrong wording, thank you for your contribution first.
>>>
>>> It really makes RK3399 the primary testing bed for 64K page size, and
>>> save me a lot of time.
>>>
>>> I'm wondering how the -EPROBE_DEFER happens.
>>> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
>>> The config works for rc4, unless something big changed in rc2, it
>>> shouldn't change much, right?
>>>
>>> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
>>> special driver.
>>> Or does it?
>>
>> Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
>> are all provided by rk808, while the dmesg shows:
>>
>> [    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
>> vin-supply from device tree
>> [    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
>> supplying 3300000uV
>> [    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
>> vin-supply from device tree
>> [    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
>> supplying 0uV
>> [    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
>> vin-supply from device tree
>> [    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
>> supplying 0uV
>> [    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
>> 3300000uV
>> [    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
>> [    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from
>> device tree
>> [    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
>> node /vdd-log failed
>> [    1.086501] vdd_log: supplied by regulator-dummy
>> [    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
>> [    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
>> [    1.419856] rk808 0-001b: failed to register 12 regulator
>> [    1.422801] rk808-regulator: probe of rk808-regulator failed with
>> error -22
>>
>> So this means something wrong with the rk808, resulting no voltage
>> provided from rk808 and screwing up the pcie controller?
> 
> Frankly PCIe is the least of your worries there - if the system PMIC is
> failing to probe then pretty much everything will be degraded: cpufreq
> won't work, some SD card modes won't work, on some boards entire
> peripherals like ethernet might not work if they're wired up to use the
> opposite I/O domain voltage setting from the SoC's power-on default.
> 
> Focusing on PCIe getting probe-deferred seems a bit like complaining
> that the carpets on the Titanic are wet ;)

That's true. Thankfully bisect lead to the cause. It's even deeper in
the core regulator code.

Commit aea6cb99703e ("regulator: resolve supply after creating
regulator") makes the RK808 unable to register.

And fix for that commit is already merged in upstream.

Sorry for the disruption.

Thanks,
Qu

> 
> Robin.
> 

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/9 下午8:00, Robin Murphy wrote:
> On 2020-11-07 13:30, Qu Wenruo wrote:
>>
>>
>> On 2020/11/7 下午8:54, Qu Wenruo wrote:
>>>
>>>
>>> On 2020/11/7 下午8:47, Robin Murphy wrote:
>>>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>>>
>>>>>
>>>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>>>> not explicitly described.
>>>>>>>
>>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>>>> ---
>>>>>>>    drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>>>> ++++++++-------------
>>>>>>>    1 file changed, 25 insertions(+), 44 deletions(-)
>>>>>>
>>>>>> Applied to pci/rockchip, thanks.
>>>>>
>>>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>>>> detect the controller anymore.
>>>>>
>>>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>>>> initialization.
>>>>
>>>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>>>> but you're missing the relevant driver - that's an issue with your
>>>> config/initrd. Being optional should only change the behaviour if the
>>>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>>>> regulator), so I don't see that it would make any difference in this
>>>> situation anyway :/
>>>>
>>>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>>>> are not related to PCIE core functionality, especially considering the
>>>>> controller used in RK3399 are mostly to provide NVME support.
>>>>
>>>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>>>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>>>> datasheet describe as "Supply voltage for PCIE". Having power is
>>>> kind of
>>>> important for the I/O circuits on all the signal pins to work.
>>>>
>>>> Now it's almost certainly true that these supplies technically
>>>> belong to
>>>> the phy rather than the controller, but it's a bit late to change the
>>>> bindings for the sake of semantics.
>>>>
>>>>> This bug makes all RK3399 users who put root fs into NVME driver
>>>>> unable
>>>>> to boot the device.
>>>>>
>>>>> I really hope some one could test the patch before affecting the end
>>>>> users or at least try to understand how most users would use the PCIE
>>>>> interface for.
>>>>
>>>> I *am* that end user in this case - I use an M.2 NVME on my board,
>>>> which
>>>> prompted me to take a look at the regulator handling here in the first
>>>> place, to see if it might be possible to shut up the annoying message
>>>> about a 12V supply that is entirely irrelevant to a board without a
>>>> full-size PCIe slot. I use a mainline-based distro, so I've been
>>>> running
>>>> this change for nearly a year since it landed in v5.5, and I'm sure
>>>> many
>>>> others have too. I've not heard of any other complaints in that time...
>>>
>>> Sorry for the wrong wording, thank you for your contribution first.
>>>
>>> It really makes RK3399 the primary testing bed for 64K page size, and
>>> save me a lot of time.
>>>
>>> I'm wondering how the -EPROBE_DEFER happens.
>>> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
>>> The config works for rc4, unless something big changed in rc2, it
>>> shouldn't change much, right?
>>>
>>> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
>>> special driver.
>>> Or does it?
>>
>> Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
>> are all provided by rk808, while the dmesg shows:
>>
>> [    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
>> vin-supply from device tree
>> [    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
>> supplying 3300000uV
>> [    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
>> vin-supply from device tree
>> [    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
>> supplying 0uV
>> [    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
>> vin-supply from device tree
>> [    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
>> supplying 0uV
>> [    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
>> 3300000uV
>> [    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
>> [    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from
>> device tree
>> [    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
>> node /vdd-log failed
>> [    1.086501] vdd_log: supplied by regulator-dummy
>> [    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
>> [    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
>> [    1.419856] rk808 0-001b: failed to register 12 regulator
>> [    1.422801] rk808-regulator: probe of rk808-regulator failed with
>> error -22
>>
>> So this means something wrong with the rk808, resulting no voltage
>> provided from rk808 and screwing up the pcie controller?
> 
> Frankly PCIe is the least of your worries there - if the system PMIC is
> failing to probe then pretty much everything will be degraded: cpufreq
> won't work, some SD card modes won't work, on some boards entire
> peripherals like ethernet might not work if they're wired up to use the
> opposite I/O domain voltage setting from the SoC's power-on default.
> 
> Focusing on PCIe getting probe-deferred seems a bit like complaining
> that the carpets on the Titanic are wet ;)

That's true. Thankfully bisect lead to the cause. It's even deeper in
the core regulator code.

Commit aea6cb99703e ("regulator: resolve supply after creating
regulator") makes the RK808 unable to register.

And fix for that commit is already merged in upstream.

Sorry for the disruption.

Thanks,
Qu

> 
> Robin.
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo]

On 2020/11/9 下午8:00, Robin Murphy wrote:
> On 2020-11-07 13:30, Qu Wenruo wrote:
>>
>>
>> On 2020/11/7 下午8:54, Qu Wenruo wrote:
>>>
>>>
>>> On 2020/11/7 下午8:47, Robin Murphy wrote:
>>>> On 2020-11-07 11:36, Qu Wenruo wrote:
>>>>>
>>>>>
>>>>> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>>>>>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>>>>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>>>>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>>>>>> sense to treat them as non-optional and rely on dummy regulators if
>>>>>>> not explicitly described.
>>>>>>>
>>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>>>> ---
>>>>>>>    drivers/pci/controller/pcie-rockchip-host.c | 69
>>>>>>> ++++++++-------------
>>>>>>>    1 file changed, 25 insertions(+), 44 deletions(-)
>>>>>>
>>>>>> Applied to pci/rockchip, thanks.
>>>>>
>>>>> Sorry, this commit is cause regression for RK3399 boards unable to
>>>>> detect the controller anymore.
>>>>>
>>>>> The 1v8 (and 0v9) is causing -517 and reject the controller
>>>>> initialization.
>>>>
>>>> That's -EPROBE_DEFER, which must mean that a regulator *is* described,
>>>> but you're missing the relevant driver - that's an issue with your
>>>> config/initrd. Being optional should only change the behaviour if the
>>>> supply is totally absent (i.e. you get -ENODEV instead of a dummy
>>>> regulator), so I don't see that it would make any difference in this
>>>> situation anyway :/
>>>>
>>>>> I'm not a PCI guy, but a quick google search shows these two voltages
>>>>> are not related to PCIE core functionality, especially considering the
>>>>> controller used in RK3399 are mostly to provide NVME support.
>>>>
>>>> Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
>>>> PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
>>>> datasheet describe as "Supply voltage for PCIE". Having power is
>>>> kind of
>>>> important for the I/O circuits on all the signal pins to work.
>>>>
>>>> Now it's almost certainly true that these supplies technically
>>>> belong to
>>>> the phy rather than the controller, but it's a bit late to change the
>>>> bindings for the sake of semantics.
>>>>
>>>>> This bug makes all RK3399 users who put root fs into NVME driver
>>>>> unable
>>>>> to boot the device.
>>>>>
>>>>> I really hope some one could test the patch before affecting the end
>>>>> users or at least try to understand how most users would use the PCIE
>>>>> interface for.
>>>>
>>>> I *am* that end user in this case - I use an M.2 NVME on my board,
>>>> which
>>>> prompted me to take a look at the regulator handling here in the first
>>>> place, to see if it might be possible to shut up the annoying message
>>>> about a 12V supply that is entirely irrelevant to a board without a
>>>> full-size PCIe slot. I use a mainline-based distro, so I've been
>>>> running
>>>> this change for nearly a year since it landed in v5.5, and I'm sure
>>>> many
>>>> others have too. I've not heard of any other complaints in that time...
>>>
>>> Sorry for the wrong wording, thank you for your contribution first.
>>>
>>> It really makes RK3399 the primary testing bed for 64K page size, and
>>> save me a lot of time.
>>>
>>> I'm wondering how the -EPROBE_DEFER happens.
>>> I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
>>> The config works for rc4, unless something big changed in rc2, it
>>> shouldn't change much, right?
>>>
>>> The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
>>> special driver.
>>> Or does it?
>>
>> Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
>> are all provided by rk808, while the dmesg shows:
>>
>> [    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
>> vin-supply from device tree
>> [    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
>> supplying 3300000uV
>> [    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
>> vin-supply from device tree
>> [    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
>> supplying 0uV
>> [    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
>> vin-supply from device tree
>> [    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
>> supplying 0uV
>> [    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
>> 3300000uV
>> [    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
>> [    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from
>> device tree
>> [    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
>> node /vdd-log failed
>> [    1.086501] vdd_log: supplied by regulator-dummy
>> [    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
>> [    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
>> [    1.419856] rk808 0-001b: failed to register 12 regulator
>> [    1.422801] rk808-regulator: probe of rk808-regulator failed with
>> error -22
>>
>> So this means something wrong with the rk808, resulting no voltage
>> provided from rk808 and screwing up the pcie controller?
> 
> Frankly PCIe is the least of your worries there - if the system PMIC is
> failing to probe then pretty much everything will be degraded: cpufreq
> won't work, some SD card modes won't work, on some boards entire
> peripherals like ethernet might not work if they're wired up to use the
> opposite I/O domain voltage setting from the SoC's power-on default.
> 
> Focusing on PCIe getting probe-deferred seems a bit like complaining
> that the carpets on the Titanic are wet ;)

That's true. Thankfully bisect lead to the cause. It's even deeper in
the core regulator code.

Commit aea6cb99703e ("regulator: resolve supply after creating
regulator") makes the RK808 unable to register.

And fix for that commit is already merged in upstream.

Sorry for the disruption.

Thanks,
Qu

> 
> Robin.
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip