[PATCH 0/2] phy: ti-pipe3: Update PCIe PHY settings

[PATCH 0/2] phy: ti-pipe3: Update PCIe PHY settings

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

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(-)

-- 
2.11.0

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

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

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;
 }
 
-- 
2.11.0

[PATCH 2/2] phy: ti-pipe3: Update pcie phy settings

[Kishon Vijay Abraham I]

Update the PCIe phy settings based on new settings available
in AM572x Technical Reference Manual[1] Revision I, revised
April 2017 in Table 26-62 "Preferred PCIe_PHY_RX SCP Register
Settings".

[1] http://www.ti.com/lit/ug/spruhz6i/spruhz6i.pdf

Cc: Vignesh R <vigneshr@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
[nsekhar@ti.com: commit message updates]
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/phy/ti/phy-ti-pipe3.c | 101 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index 0e564f32749f..68ce4a082b9b 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -68,6 +68,40 @@
 #define PCIE_PCS_MASK			0xFF0000
 #define PCIE_PCS_DELAY_COUNT_SHIFT	0x10
 
+#define PCIEPHYRX_ANA_PROGRAMMABILITY	0x0000000C
+#define INTERFACE_MASK			GENMASK(31, 27)
+#define INTERFACE_SHIFT			27
+#define LOSD_MASK			GENMASK(17, 14)
+#define LOSD_SHIFT			14
+#define MEM_PLLDIV			GENMASK(6, 5)
+
+#define PCIEPHYRX_TRIM			0x0000001C
+#define MEM_DLL_TRIM_SEL		GENMASK(31, 30)
+#define MEM_DLL_TRIM_SHIFT		30
+
+#define PCIEPHYRX_DLL			0x00000024
+#define MEM_DLL_PHINT_RATE		GENMASK(31, 30)
+
+#define PCIEPHYRX_DIGITAL_MODES		0x00000028
+#define MEM_CDR_FASTLOCK		BIT(23)
+#define MEM_CDR_LBW			GENMASK(22, 21)
+#define MEM_CDR_STEPCNT			GENMASK(20, 19)
+#define MEM_CDR_STL_MASK		GENMASK(18, 16)
+#define MEM_CDR_STL_SHIFT		16
+#define MEM_CDR_THR_MASK		GENMASK(15, 13)
+#define MEM_CDR_THR_SHIFT		13
+#define MEM_CDR_THR_MODE		BIT(12)
+#define MEM_CDR_CDR_2NDO_SDM_MODE	BIT(11)
+#define MEM_OVRD_HS_RATE		BIT(26)
+
+#define PCIEPHYRX_EQUALIZER		0x00000038
+#define MEM_EQLEV			GENMASK(31, 16)
+#define MEM_EQFTC			GENMASK(15, 11)
+#define MEM_EQCTL			GENMASK(10, 7)
+#define MEM_EQCTL_SHIFT			7
+#define MEM_OVRD_EQLEV			BIT(2)
+#define MEM_OVRD_EQFTC			BIT(1)
+
 /*
  * This is an Empirical value that works, need to confirm the actual
  * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status
@@ -91,6 +125,8 @@ struct pipe3_dpll_map {
 
 struct ti_pipe3 {
 	void __iomem		*pll_ctrl_base;
+	void __iomem		*phy_rx;
+	void __iomem		*phy_tx;
 	struct device		*dev;
 	struct device		*control_dev;
 	struct clk		*wkupclk;
@@ -261,6 +297,37 @@ static int ti_pipe3_dpll_program(struct ti_pipe3 *phy)
 	return ti_pipe3_dpll_wait_lock(phy);
 }
 
+static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
+{
+	u32 val;
+
+	val = ti_pipe3_readl(phy->phy_rx, PCIEPHYRX_ANA_PROGRAMMABILITY);
+	val &= ~(INTERFACE_MASK | LOSD_MASK | MEM_PLLDIV);
+	val = (0x1 << INTERFACE_SHIFT | 0xA << LOSD_SHIFT);
+	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_ANA_PROGRAMMABILITY, val);
+
+	val = ti_pipe3_readl(phy->phy_rx, PCIEPHYRX_DIGITAL_MODES);
+	val &= ~(MEM_CDR_STEPCNT | MEM_CDR_STL_MASK | MEM_CDR_THR_MASK |
+		 MEM_CDR_CDR_2NDO_SDM_MODE | MEM_OVRD_HS_RATE);
+	val |= (MEM_CDR_FASTLOCK | MEM_CDR_LBW | 0x3 << MEM_CDR_STL_SHIFT |
+		0x1 << MEM_CDR_THR_SHIFT | MEM_CDR_THR_MODE);
+	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_DIGITAL_MODES, val);
+
+	val = ti_pipe3_readl(phy->phy_rx, PCIEPHYRX_TRIM);
+	val &= ~MEM_DLL_TRIM_SEL;
+	val |= 0x2 << MEM_DLL_TRIM_SHIFT;
+	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_TRIM, val);
+
+	val = ti_pipe3_readl(phy->phy_rx, PCIEPHYRX_DLL);
+	val |= MEM_DLL_PHINT_RATE;
+	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_DLL, val);
+
+	val = ti_pipe3_readl(phy->phy_rx, PCIEPHYRX_EQUALIZER);
+	val &= ~(MEM_EQLEV | MEM_EQCTL | MEM_OVRD_EQLEV | MEM_OVRD_EQFTC);
+	val |= MEM_EQFTC | 0x1 << MEM_EQCTL_SHIFT;
+	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
+}
+
 static int ti_pipe3_init(struct phy *x)
 {
 	struct ti_pipe3 *phy = phy_get_drvdata(x);
@@ -282,7 +349,12 @@ static int ti_pipe3_init(struct phy *x)
 		val = 0x96 << OMAP_CTRL_PCIE_PCS_DELAY_COUNT_SHIFT;
 		ret = regmap_update_bits(phy->pcs_syscon, phy->pcie_pcs_reg,
 					 PCIE_PCS_MASK, val);
-		return ret;
+		if (ret)
+			return ret;
+
+		ti_pipe3_calibrate(phy);
+
+		return 0;
 	}
 
 	/* Bring it out of IDLE if it is IDLE */
@@ -513,6 +585,29 @@ static int ti_pipe3_get_sysctrl(struct ti_pipe3 *phy)
 	return 0;
 }
 
+static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)
+{
+	struct resource *res;
+	struct device *dev = phy->dev;
+	struct device_node *node = dev->of_node;
+	struct platform_device *pdev = to_platform_device(dev);
+
+	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))
+		return 0;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "phy_rx");
+	phy->phy_rx = devm_ioremap_resource(dev, res);
+	if (IS_ERR(phy->phy_rx))
+		return PTR_ERR(phy->phy_rx);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "phy_tx");
+	phy->phy_tx = devm_ioremap_resource(dev, res);
+
+	return PTR_ERR_OR_ZERO(phy->phy_tx);
+}
+
 static int ti_pipe3_get_pll_base(struct ti_pipe3 *phy)
 {
 	struct resource *res;
@@ -559,6 +654,10 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = ti_pipe3_get_tx_rx_base(phy);
+	if (ret)
+		return ret;
+
 	ret = ti_pipe3_get_sysctrl(phy);
 	if (ret)
 		return ret;
-- 
2.11.0

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

[Roger Quadros]


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;
>  }
>  
> 

-- 
cheers,
-roger

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

[Kishon Vijay Abraham I]

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

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

[Roger Quadros]


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
> 

-- 
cheers,
-roger

Re: [PATCH 0/2] phy: ti-pipe3: Update PCIe PHY settings

[Bjorn Helgaas]

On Mon, Oct 09, 2017 at 02:33:36PM +0530, Kishon Vijay Abraham I wrote:
> 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

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 <bhelgaas@google.com>

Let me know if you'd like me to take one or both of them.

>   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(-)
> 
> -- 
> 2.11.0
> 

Re: [PATCH 0/2] phy: ti-pipe3: Update PCIe PHY settings

[Kishon Vijay Abraham I]

On Wednesday 18 October 2017 01:01 AM, Bjorn Helgaas wrote:
> On Mon, Oct 09, 2017 at 02:33:36PM +0530, Kishon Vijay Abraham I wrote:
>> 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
> 
> 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 <bhelgaas@google.com>

Yeah both should be merged together. I'll make the change you suggested, add
your Ack and merge myself.

Thanks
Kishon

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

[Kishon Vijay Abraham I]

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

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

[Roger Quadros]

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
> 

-- 
cheers,
-roger

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