phy: ti-pipe3: Update PCIe PHY settings

PCI core access configuration space registers in resume_noirq callbacks. In the case of dra7xx, PIPE3 PHY connected to PCIe controller has to be enabled before accessing configuration space registers. Since PIPE3 PHY is enabled by only configuring control module registers, no aborts has been observed so far (though during noirq stage, interface clock of PIPE3 PHY is not enabled).

With new TRM updates, PIPE3 PHY has to be initialized (PIPE3 PHY registers has to be accessed) as well which requires the interface clock of PIPE3 PHY to be enabled. The interface clock of PIPE3 PHY is derived from OCP2SCP and hence PCIe PHY is modeled as a child of OCP2SCP. Since pm_runtime is not enabled during noirq stage, pm_runtime_get_sync done in phy_init doesn't enable OCP2SCP clocks resulting in abort when PIPE3 PHY registers are accessed.

Create a function dependency between PCIe and PHY here to make sure PCIe is suspended before PCIe PHY/OCP2SCP and resumed after PCIe PHY/OCP2SCP.

Suggested-by Grygorii Strashko <[email protected]>

Signed-off-by Kishon Vijay Abraham I <[email protected]>

Signed-off-by Sekhar Nori <[email protected]>

---
 drivers/pci/dwc/pci-dra7xx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/pci/dwc/pci-dra7xx.c +16 -0

index 34427a6a15af..362607f727ee 100644
@@ -11,6 +11,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>

@@ -594,6 +595,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 	int i;
 	int phy_count;
 	struct phy **phy;
+	struct device_link **link;
 	void __iomem *base;
 	struct resource *res;
 	struct dw_pcie *pci;

@@ -649,11 +651,21 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 	if (!phy)
 		return -ENOMEM;
 
+	link = devm_kzalloc(dev, sizeof(*link) * phy_count, GFP_KERNEL);
+	if (!link)
+		return -ENOMEM;
+
 	for (i = 0; i < phy_count; i++) {
 		snprintf(name, sizeof(name), "pcie-phy%d", i);
 		phy[i] = devm_phy_get(dev, name);
 		if (IS_ERR(phy[i]))
 			return PTR_ERR(phy[i]);
+
+		link[i] = device_link_add(dev, &phy[i]->dev, DL_FLAG_STATELESS);
+		if (!link[i]) {
+			ret = -EINVAL;
+			goto err_link;
+		}
 	}
 
 	dra7xx->base = base;

@@ -732,6 +744,10 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 	dra7xx_pcie_disable_phy(dra7xx);
 
+err_link:
+	while (--i >= 0)
+		device_link_del(link[i]);
+
 	return ret;
 }
 

Quoted 7 lines · Kishon Vijay Abraham I

PCI core access configuration space registers in resume_noirq callbacks. In the case of dra7xx, PIPE3 PHY connected to PCIe controller has to be enabled before accessing configuration space registers. Since PIPE3 PHY is enabled by only configuring control module registers, no aborts has been observed so far (though during noirq stage, interface clock of PIPE3 PHY is not enabled).

Quoted 8 lines

With new TRM updates, PIPE3 PHY has to be initialized (PIPE3 PHY registers has to be accessed) as well which requires the interface clock of PIPE3 PHY to be enabled. The interface clock of PIPE3 PHY is derived from OCP2SCP and hence PCIe PHY is modeled as a child of OCP2SCP. Since pm_runtime is not enabled during noirq stage, pm_runtime_get_sync done in phy_init doesn't enable OCP2SCP clocks resulting in abort when PIPE3 PHY registers are accessed.

This could be the case not only with PCIe but even with USB and SATA?

Maybe today it doesn't complain because USB/SATA is being resumed (by chance) after PHY?

Roger,

Quoted 17 lines · Roger Quadros

Quoted 15 lines · Kishon Vijay Abraham I

PCI core access configuration space registers in resume_noirq callbacks. In the case of dra7xx, PIPE3 PHY connected to PCIe controller has to be enabled before accessing configuration space registers. Since PIPE3 PHY is enabled by only configuring control module registers, no aborts has been observed so far (though during noirq stage, interface clock of PIPE3 PHY is not enabled).

With new TRM updates, PIPE3 PHY has to be initialized (PIPE3 PHY registers has to be accessed) as well which requires the interface clock of PIPE3 PHY to be enabled. The interface clock of PIPE3 PHY is derived from OCP2SCP and hence PCIe PHY is modeled as a child of OCP2SCP. Since pm_runtime is not enabled during noirq stage, pm_runtime_get_sync done in phy_init doesn't enable OCP2SCP clocks resulting in abort when PIPE3 PHY registers are accessed.

Quoted 1 line

This could be the case not only with PCIe but even with USB and SATA?

right, if USB/SATA driver has no_irq callbacks then the same issue might occur.

But looks like none of them uses no_irq callbacks.

Quoted 1 line

Maybe today it doesn't complain because USB/SATA is being resumed (by chance) after PHY?

But once pm_runtime is enabled, USB/SATA can enable PHY which in turn will enable OCP2SCP and PHY can be accessed without any issues.

Thanks
Kishon

Quoted 20 lines · Kishon Vijay Abraham I

Roger,

Quoted 17 lines · Roger Quadros

Quoted 15 lines · Kishon Vijay Abraham I

PCI core access configuration space registers in resume_noirq callbacks. In the case of dra7xx, PIPE3 PHY connected to PCIe controller has to be enabled before accessing configuration space registers. Since PIPE3 PHY is enabled by only configuring control module registers, no aborts has been observed so far (though during noirq stage, interface clock of PIPE3 PHY is not enabled).

With new TRM updates, PIPE3 PHY has to be initialized (PIPE3 PHY registers has to be accessed) as well which requires the interface clock of PIPE3 PHY to be enabled. The interface clock of PIPE3 PHY is derived from OCP2SCP and hence PCIe PHY is modeled as a child of OCP2SCP. Since pm_runtime is not enabled during noirq stage, pm_runtime_get_sync done in phy_init doesn't enable OCP2SCP clocks resulting in abort when PIPE3 PHY registers are accessed.

Quoted 3 lines

Quoted 1 line

This could be the case not only with PCIe but even with USB and SATA?

right, if USB/SATA driver has no_irq callbacks then the same issue might occur.

OK. Is this something that we should document in Documentation/phy.txt?

Hi,

Quoted 21 lines · Roger Quadros

Quoted 20 lines · Kishon Vijay Abraham I

Roger,

Quoted 17 lines · Roger Quadros

Quoted 15 lines · Kishon Vijay Abraham I

PCI core access configuration space registers in resume_noirq callbacks. In the case of dra7xx, PIPE3 PHY connected to PCIe controller has to be enabled before accessing configuration space registers. Since PIPE3 PHY is enabled by only configuring control module registers, no aborts has been observed so far (though during noirq stage, interface clock of PIPE3 PHY is not enabled).

With new TRM updates, PIPE3 PHY has to be initialized (PIPE3 PHY registers has to be accessed) as well which requires the interface clock of PIPE3 PHY to be enabled. The interface clock of PIPE3 PHY is derived from OCP2SCP and hence PCIe PHY is modeled as a child of OCP2SCP. Since pm_runtime is not enabled during noirq stage, pm_runtime_get_sync done in phy_init doesn't enable OCP2SCP clocks resulting in abort when PIPE3 PHY registers are accessed.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

This could be the case not only with PCIe but even with USB and SATA?

right, if USB/SATA driver has no_irq callbacks then the same issue might occur.

OK. Is this something that we should document in Documentation/phy.txt?

Yeah it could be but the dependency is not required for a lot of phy consumer drivers.

Do you think it makes sense to create functional dependency between phy and it's consumers in phy_get?

Thanks
Kishon

This was supposed to only update ti-pipe3 PHY registers. However because of the way the ti-pipe3 PHY, OCP2SCP and PCIe controller are connected in dra7xx where

PCIe controller --------------> ti-pipe3 PHY --------------> OCP2SCP
depends on depends on

updating ti-pipe3 PHY registers results in an abort.

Though the dependency between ti-pipe3 PHY and OCP2SCP is created (OCP2SCP is parent of ti-pipe3 PHY), and enabling ti-pipe3 PHY clocks should in turn enable OCP2SCP (with the help of pm_runtime framework), this doesn't work in no_irq stage since pm_runtime is disabled during no_irq stage. Since pci-dra7xx enables/initializes ti-pipe3 phy in no_irq stage, OCP2SCP is not enabled resulting in an abort with ti-pipe3 PHY registers are accessed.

In order to solve this a functional dependency is created between PCIe and ti-pipe3 PHY so that PCIe is suspended before PHY/OCP2SCP and resumed after PCIe PHY/OCP2SCP

Kishon Vijay Abraham I (2):
PCI: dwc: dra7xx: Create functional dependency between PCIe and PHY

phy: ti-pipe3: Update pcie phy settings

 drivers/pci/dwc/pci-dra7xx.c  |  16 +++++++
 drivers/phy/ti/phy-ti-pipe3.c | 101 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 116 insertions(+), 1 deletion(-)

Signature · 1 line

2.11.0

Quoted 17 lines · Kishon Vijay Abraham I

This was supposed to only update ti-pipe3 PHY registers. However because of the way the ti-pipe3 PHY, OCP2SCP and PCIe controller are connected in dra7xx where

PCIe controller --------------> ti-pipe3 PHY --------------> OCP2SCP
depends on depends on

updating ti-pipe3 PHY registers results in an abort.

Though the dependency between ti-pipe3 PHY and OCP2SCP is created (OCP2SCP is parent of ti-pipe3 PHY), and enabling ti-pipe3 PHY clocks should in turn enable OCP2SCP (with the help of pm_runtime framework), this doesn't work in no_irq stage since pm_runtime is disabled during no_irq stage. Since pci-dra7xx enables/initializes ti-pipe3 phy in no_irq stage, OCP2SCP is not enabled resulting in an abort with ti-pipe3 PHY registers are accessed.

Quoted 6 lines

In order to solve this a functional dependency is created between PCIe and ti-pipe3 PHY so that PCIe is suspended before PHY/OCP2SCP and resumed after PCIe PHY/OCP2SCP

Kishon Vijay Abraham I (2):
PCI: dwc: dra7xx: Create functional dependency between PCIe and PHY

s/PCI: dwc: dra7xx: Create functional dependency between PCIe and PHY/
PCI: dra7xx: Create functional dependency between PCIe and PHY/

Should these be merged together? If it makes sense for you to merge them together, here's my ack for the PCI piece:

Acked-by Bjorn Helgaas <[email protected]>