diff mbox

[1/2] PCI: dwc: dra7xx: Create functional dependency between PCIe and PHY

Message ID 20171009090338.26033-2-kishon@ti.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Kishon Vijay Abraham I Oct. 9, 2017, 9:03 a.m. UTC 
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 <grygorii.strashko@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/pci/dwc/pci-dra7xx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Roger Quadros Oct. 10, 2017, 7:19 a.m. UTC | #1 

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 09/10/17 12:03, Kishon Vijay Abraham I wrote:
> 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.

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?

> 
> 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 <grygorii.strashko@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> ---
>  drivers/pci/dwc/pci-dra7xx.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 34427a6a15af..362607f727ee 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -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;
>  }
>  
>
Kishon Vijay Abraham I Oct. 10, 2017, 7:42 a.m. UTC | #2 
Roger,

On Tuesday 10 October 2017 12:49 PM, Roger Quadros wrote:
> On 09/10/17 12:03, Kishon Vijay Abraham I wrote:
>> 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.
> 
> 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.
> 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
Roger Quadros Oct. 10, 2017, 7:59 a.m. UTC | #3 

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 10/10/17 10:42, Kishon Vijay Abraham I wrote:
> Roger,
> 
> On Tuesday 10 October 2017 12:49 PM, Roger Quadros wrote:
>> On 09/10/17 12:03, Kishon Vijay Abraham I wrote:
>>> 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.
>>
>> 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?

> But looks like none of them uses no_irq callbacks.

>> 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
>
Kishon Vijay Abraham I Oct. 18, 2017, 12:12 p.m. UTC | #4 
Hi,

On Tuesday 10 October 2017 01:29 PM, Roger Quadros wrote:
> On 10/10/17 10:42, Kishon Vijay Abraham I wrote:
>> Roger,
>>
>> On Tuesday 10 October 2017 12:49 PM, Roger Quadros wrote:
>>> On 09/10/17 12:03, Kishon Vijay Abraham I wrote:
>>>> 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.
>>>
>>> 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
Roger Quadros Oct. 19, 2017, 9:46 a.m. UTC | #5 
Hi,

On 18/10/17 15:12, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Tuesday 10 October 2017 01:29 PM, Roger Quadros wrote:
>> On 10/10/17 10:42, Kishon Vijay Abraham I wrote:
>>> Roger,
>>>
>>> On Tuesday 10 October 2017 12:49 PM, Roger Quadros wrote:
>>>> On 09/10/17 12:03, Kishon Vijay Abraham I wrote:
>>>>> 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.
>>>>
>>>> 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.

But they could add a no_irq callback in the future and then things break.

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

I think making it generic makes more sense without placing restrictions on users.

> 
> Thanks
> Kishon
>
diff mbox

Patch

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 34427a6a15af..362607f727ee 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -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;
 }