[PATCH v2 0/3] phy: ti-pipe3: dra7: sata: allow suspend to RAM (core-retention)

[PATCH v2 0/3] phy: ti-pipe3: dra7: sata: allow suspend to RAM (core-retention)

[Roger Quadros]

Hi,

We fix 2 things here to ensure that PHY clocks are disabled during
system suspend.

1) Stop relying on pm_runtime ops to disable our clocks as they don't
fit in well with the PHY framework usage scenario. This ensures that all clocks are
disabled when phy_exit() is called.

2) Implement workaround for SATA errata i783 (SATA Lockup After SATA DPLL Unlock/Relock)
so that we can now turn off sata_refclk to support suspend-to-ram without preventing
core-retention.

Changelog:
v2:
- Fixed pcie disable_clocks. Addressed review comments.

cheers,
-roger

Roger Quadros (3):
  phy: ti-pipe3: fix suspend
  phy: ti-pipe3: i783 workaround for SATA lockup after dpll
    unlock/relock
  ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

 Documentation/devicetree/bindings/phy/ti-phy.txt |  16 ++
 arch/arm/boot/dts/dra7.dtsi                      |   1 +
 drivers/phy/phy-ti-pipe3.c                       | 217 +++++++++--------------
 3 files changed, 105 insertions(+), 129 deletions(-)

-- 
2.1.4

[PATCH v2 0/3] phy: ti-pipe3: dra7: sata: allow suspend to RAM (core-retention)

[Roger Quadros]

Hi,

We fix 2 things here to ensure that PHY clocks are disabled during
system suspend.

1) Stop relying on pm_runtime ops to disable our clocks as they don't
fit in well with the PHY framework usage scenario. This ensures that all clocks are
disabled when phy_exit() is called.

2) Implement workaround for SATA errata i783 (SATA Lockup After SATA DPLL Unlock/Relock)
so that we can now turn off sata_refclk to support suspend-to-ram without preventing
core-retention.

Changelog:
v2:
- Fixed pcie disable_clocks. Addressed review comments.

cheers,
-roger

Roger Quadros (3):
  phy: ti-pipe3: fix suspend
  phy: ti-pipe3: i783 workaround for SATA lockup after dpll
    unlock/relock
  ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

 Documentation/devicetree/bindings/phy/ti-phy.txt |  16 ++
 arch/arm/boot/dts/dra7.dtsi                      |   1 +
 drivers/phy/phy-ti-pipe3.c                       | 217 +++++++++--------------
 3 files changed, 105 insertions(+), 129 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] phy: ti-pipe3: fix suspend

[Roger Quadros]

Relying on PM-ops for shutting down PHY clocks was a
bad idea since the users (e.g. PCIe/SATA) might not
have been suspended by then.

The main culprit for not shutting down the clocks was
the stray pm_runtime_get() call in probe.

Fix the whole thing in the right way by getting rid
of that pm_runtime_get() call from probe and
removing all PM-ops. It is the sole responsibility
of the PHY user to properly turn OFF and de-initialize
the PHY as part of its suspend routine.

As PHY core serializes init/exit we don't need
to use a spinlock in this driver. So get rid of it.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/phy/phy-ti-pipe3.c | 172 +++++++++++----------------------------------
 1 file changed, 41 insertions(+), 131 deletions(-)

diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
index 53f295c..3510b81 100644
--- a/drivers/phy/phy-ti-pipe3.c
+++ b/drivers/phy/phy-ti-pipe3.c
@@ -28,7 +28,6 @@
 #include <linux/delay.h>
 #include <linux/phy/omap_control_phy.h>
 #include <linux/of_platform.h>
-#include <linux/spinlock.h>
 
 #define	PLL_STATUS		0x00000004
 #define	PLL_GO			0x00000008
@@ -83,10 +82,6 @@ struct ti_pipe3 {
 	struct clk		*refclk;
 	struct clk		*div_clk;
 	struct pipe3_dpll_map	*dpll_map;
-	bool			enabled;
-	spinlock_t		lock;	/* serialize clock enable/disable */
-	/* the below flag is needed specifically for SATA */
-	bool			refclk_enabled;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {
@@ -137,6 +132,9 @@ static struct pipe3_dpll_params *ti_pipe3_get_dpll_params(struct ti_pipe3 *phy)
 	return NULL;
 }
 
+static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy);
+static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy);
+
 static int ti_pipe3_power_off(struct phy *x)
 {
 	struct ti_pipe3 *phy = phy_get_drvdata(x);
@@ -217,6 +215,7 @@ static int ti_pipe3_init(struct phy *x)
 	u32 val;
 	int ret = 0;
 
+	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table
@@ -250,33 +249,35 @@ static int ti_pipe3_exit(struct phy *x)
 	u32 val;
 	unsigned long timeout;
 
-	/* SATA DPLL can't be powered down due to Errata i783 and PCIe
-	 * does not have internal DPLL
-	 */
-	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata") ||
-	    of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie"))
+	/* SATA DPLL can't be powered down due to Errata i783 */
+	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata"))
 		return 0;
 
-	/* Put DPLL in IDLE mode */
-	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
-	val |= PLL_IDLE;
-	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
-
-	/* wait for LDO and Oscillator to power down */
-	timeout = jiffies + msecs_to_jiffies(PLL_IDLE_TIME);
-	do {
-		cpu_relax();
-		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
-		if ((val & PLL_TICOPWDN) && (val & PLL_LDOPWDN))
-			break;
-	} while (!time_after(jiffies, timeout));
+	/* PCIe doesn't have internal DPLL */
+	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+		/* Put DPLL in IDLE mode */
+		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
+		val |= PLL_IDLE;
+		ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
 
-	if (!(val & PLL_TICOPWDN) || !(val & PLL_LDOPWDN)) {
-		dev_err(phy->dev, "Failed to power down: PLL_STATUS 0x%x\n",
-			val);
-		return -EBUSY;
+		/* wait for LDO and Oscillator to power down */
+		timeout = jiffies + msecs_to_jiffies(PLL_IDLE_TIME);
+		do {
+			cpu_relax();
+			val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
+			if ((val & PLL_TICOPWDN) && (val & PLL_LDOPWDN))
+				break;
+		} while (!time_after(jiffies, timeout));
+
+		if (!(val & PLL_TICOPWDN) || !(val & PLL_LDOPWDN)) {
+			dev_err(phy->dev, "Failed to power down: PLL_STATUS 0x%x\n",
+				val);
+			return -EBUSY;
+		}
 	}
 
+	ti_pipe3_disable_clocks(phy);
+
 	return 0;
 }
 static struct phy_ops ops = {
@@ -306,7 +307,6 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	phy->dev		= &pdev->dev;
-	spin_lock_init(&phy->lock);
 
 	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
 		match = of_match_device(ti_pipe3_id_table, &pdev->dev);
@@ -402,6 +402,10 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, phy);
 	pm_runtime_enable(phy->dev);
+	/* Prevent auto-disable of refclk for SATA PHY due to Errata i783 */
+	if (of_device_is_compatible(node, "ti,phy-pipe3-sata"))
+		if (!IS_ERR(phy->refclk))
+			clk_prepare_enable(phy->refclk);
 
 	generic_phy = devm_phy_create(phy->dev, NULL, &ops);
 	if (IS_ERR(generic_phy))
@@ -413,63 +417,33 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	if (IS_ERR(phy_provider))
 		return PTR_ERR(phy_provider);
 
-	pm_runtime_get(&pdev->dev);
-
 	return 0;
 }
 
 static int ti_pipe3_remove(struct platform_device *pdev)
 {
-	if (!pm_runtime_suspended(&pdev->dev))
-		pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int ti_pipe3_enable_refclk(struct ti_pipe3 *phy)
+static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
 {
-	if (!IS_ERR(phy->refclk) && !phy->refclk_enabled) {
-		int ret;
+	int ret = 0;
 
+	if (!IS_ERR(phy->refclk)) {
 		ret = clk_prepare_enable(phy->refclk);
 		if (ret) {
 			dev_err(phy->dev, "Failed to enable refclk %d\n", ret);
 			return ret;
 		}
-		phy->refclk_enabled = true;
 	}
 
-	return 0;
-}
-
-static void ti_pipe3_disable_refclk(struct ti_pipe3 *phy)
-{
-	if (!IS_ERR(phy->refclk))
-		clk_disable_unprepare(phy->refclk);
-
-	phy->refclk_enabled = false;
-}
-
-static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
-{
-	int ret = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&phy->lock, flags);
-	if (phy->enabled)
-		goto err1;
-
-	ret = ti_pipe3_enable_refclk(phy);
-	if (ret)
-		goto err1;
-
 	if (!IS_ERR(phy->wkupclk)) {
 		ret = clk_prepare_enable(phy->wkupclk);
 		if (ret) {
 			dev_err(phy->dev, "Failed to enable wkupclk %d\n", ret);
-			goto err2;
+			goto disable_refclk;
 		}
 	}
 
@@ -477,96 +451,33 @@ static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
 		ret = clk_prepare_enable(phy->div_clk);
 		if (ret) {
 			dev_err(phy->dev, "Failed to enable div_clk %d\n", ret);
-			goto err3;
+			goto disable_wkupclk;
 		}
 	}
 
-	phy->enabled = true;
-	spin_unlock_irqrestore(&phy->lock, flags);
 	return 0;
 
-err3:
+disable_wkupclk:
 	if (!IS_ERR(phy->wkupclk))
 		clk_disable_unprepare(phy->wkupclk);
 
-err2:
+disable_refclk:
 	if (!IS_ERR(phy->refclk))
 		clk_disable_unprepare(phy->refclk);
 
-	ti_pipe3_disable_refclk(phy);
-err1:
-	spin_unlock_irqrestore(&phy->lock, flags);
 	return ret;
 }
 
 static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&phy->lock, flags);
-	if (!phy->enabled) {
-		spin_unlock_irqrestore(&phy->lock, flags);
-		return;
-	}
-
 	if (!IS_ERR(phy->wkupclk))
 		clk_disable_unprepare(phy->wkupclk);
-	/* Don't disable refclk for SATA PHY due to Errata i783 */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata"))
-		ti_pipe3_disable_refclk(phy);
+	if (!IS_ERR(phy->refclk))
+		clk_disable_unprepare(phy->refclk);
 	if (!IS_ERR(phy->div_clk))
 		clk_disable_unprepare(phy->div_clk);
-	phy->enabled = false;
-	spin_unlock_irqrestore(&phy->lock, flags);
-}
-
-static int ti_pipe3_runtime_suspend(struct device *dev)
-{
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-
-	ti_pipe3_disable_clocks(phy);
-	return 0;
 }
 
-static int ti_pipe3_runtime_resume(struct device *dev)
-{
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-	int ret = 0;
-
-	ret = ti_pipe3_enable_clocks(phy);
-	return ret;
-}
-
-static int ti_pipe3_suspend(struct device *dev)
-{
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-
-	ti_pipe3_disable_clocks(phy);
-	return 0;
-}
-
-static int ti_pipe3_resume(struct device *dev)
-{
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-	int ret;
-
-	ret = ti_pipe3_enable_clocks(phy);
-	if (ret)
-		return ret;
-
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-	return 0;
-}
-#endif
-
-static const struct dev_pm_ops ti_pipe3_pm_ops = {
-	SET_RUNTIME_PM_OPS(ti_pipe3_runtime_suspend,
-			   ti_pipe3_runtime_resume, NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(ti_pipe3_suspend, ti_pipe3_resume)
-};
-
 static const struct of_device_id ti_pipe3_id_table[] = {
 	{
 		.compatible = "ti,phy-usb3",
@@ -592,7 +503,6 @@ static struct platform_driver ti_pipe3_driver = {
 	.remove		= ti_pipe3_remove,
 	.driver		= {
 		.name	= "ti-pipe3",
-		.pm	= &ti_pipe3_pm_ops,
 		.of_match_table = ti_pipe3_id_table,
 	},
 };
-- 
2.1.4

[PATCH v2 1/3] phy: ti-pipe3: fix suspend

[Roger Quadros]

Relying on PM-ops for shutting down PHY clocks was a
bad idea since the users (e.g. PCIe/SATA) might not
have been suspended by then.

The main culprit for not shutting down the clocks was
the stray pm_runtime_get() call in probe.

Fix the whole thing in the right way by getting rid
of that pm_runtime_get() call from probe and
removing all PM-ops. It is the sole responsibility
of the PHY user to properly turn OFF and de-initialize
the PHY as part of its suspend routine.

As PHY core serializes init/exit we don't need
to use a spinlock in this driver. So get rid of it.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/phy/phy-ti-pipe3.c | 172 +++++++++++----------------------------------
 1 file changed, 41 insertions(+), 131 deletions(-)

diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
index 53f295c..3510b81 100644
--- a/drivers/phy/phy-ti-pipe3.c
+++ b/drivers/phy/phy-ti-pipe3.c
@@ -28,7 +28,6 @@
 #include <linux/delay.h>
 #include <linux/phy/omap_control_phy.h>
 #include <linux/of_platform.h>
-#include <linux/spinlock.h>
 
 #define	PLL_STATUS		0x00000004
 #define	PLL_GO			0x00000008
@@ -83,10 +82,6 @@ struct ti_pipe3 {
 	struct clk		*refclk;
 	struct clk		*div_clk;
 	struct pipe3_dpll_map	*dpll_map;
-	bool			enabled;
-	spinlock_t		lock;	/* serialize clock enable/disable */
-	/* the below flag is needed specifically for SATA */
-	bool			refclk_enabled;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {
@@ -137,6 +132,9 @@ static struct pipe3_dpll_params *ti_pipe3_get_dpll_params(struct ti_pipe3 *phy)
 	return NULL;
 }
 
+static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy);
+static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy);
+
 static int ti_pipe3_power_off(struct phy *x)
 {
 	struct ti_pipe3 *phy = phy_get_drvdata(x);
@@ -217,6 +215,7 @@ static int ti_pipe3_init(struct phy *x)
 	u32 val;
 	int ret = 0;
 
+	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table
@@ -250,33 +249,35 @@ static int ti_pipe3_exit(struct phy *x)
 	u32 val;
 	unsigned long timeout;
 
-	/* SATA DPLL can't be powered down due to Errata i783 and PCIe
-	 * does not have internal DPLL
-	 */
-	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata") ||
-	    of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie"))
+	/* SATA DPLL can't be powered down due to Errata i783 */
+	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata"))
 		return 0;
 
-	/* Put DPLL in IDLE mode */
-	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
-	val |= PLL_IDLE;
-	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
-
-	/* wait for LDO and Oscillator to power down */
-	timeout = jiffies + msecs_to_jiffies(PLL_IDLE_TIME);
-	do {
-		cpu_relax();
-		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
-		if ((val & PLL_TICOPWDN) && (val & PLL_LDOPWDN))
-			break;
-	} while (!time_after(jiffies, timeout));
+	/* PCIe doesn't have internal DPLL */
+	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+		/* Put DPLL in IDLE mode */
+		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
+		val |= PLL_IDLE;
+		ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
 
-	if (!(val & PLL_TICOPWDN) || !(val & PLL_LDOPWDN)) {
-		dev_err(phy->dev, "Failed to power down: PLL_STATUS 0x%x\n",
-			val);
-		return -EBUSY;
+		/* wait for LDO and Oscillator to power down */
+		timeout = jiffies + msecs_to_jiffies(PLL_IDLE_TIME);
+		do {
+			cpu_relax();
+			val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
+			if ((val & PLL_TICOPWDN) && (val & PLL_LDOPWDN))
+				break;
+		} while (!time_after(jiffies, timeout));
+
+		if (!(val & PLL_TICOPWDN) || !(val & PLL_LDOPWDN)) {
+			dev_err(phy->dev, "Failed to power down: PLL_STATUS 0x%x\n",
+				val);
+			return -EBUSY;
+		}
 	}
 
+	ti_pipe3_disable_clocks(phy);
+
 	return 0;
 }
 static struct phy_ops ops = {
@@ -306,7 +307,6 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	phy->dev		= &pdev->dev;
-	spin_lock_init(&phy->lock);
 
 	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
 		match = of_match_device(ti_pipe3_id_table, &pdev->dev);
@@ -402,6 +402,10 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, phy);
 	pm_runtime_enable(phy->dev);
+	/* Prevent auto-disable of refclk for SATA PHY due to Errata i783 */
+	if (of_device_is_compatible(node, "ti,phy-pipe3-sata"))
+		if (!IS_ERR(phy->refclk))
+			clk_prepare_enable(phy->refclk);
 
 	generic_phy = devm_phy_create(phy->dev, NULL, &ops);
 	if (IS_ERR(generic_phy))
@@ -413,63 +417,33 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	if (IS_ERR(phy_provider))
 		return PTR_ERR(phy_provider);
 
-	pm_runtime_get(&pdev->dev);
-
 	return 0;
 }
 
 static int ti_pipe3_remove(struct platform_device *pdev)
 {
-	if (!pm_runtime_suspended(&pdev->dev))
-		pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int ti_pipe3_enable_refclk(struct ti_pipe3 *phy)
+static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
 {
-	if (!IS_ERR(phy->refclk) && !phy->refclk_enabled) {
-		int ret;
+	int ret = 0;
 
+	if (!IS_ERR(phy->refclk)) {
 		ret = clk_prepare_enable(phy->refclk);
 		if (ret) {
 			dev_err(phy->dev, "Failed to enable refclk %d\n", ret);
 			return ret;
 		}
-		phy->refclk_enabled = true;
 	}
 
-	return 0;
-}
-
-static void ti_pipe3_disable_refclk(struct ti_pipe3 *phy)
-{
-	if (!IS_ERR(phy->refclk))
-		clk_disable_unprepare(phy->refclk);
-
-	phy->refclk_enabled = false;
-}
-
-static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
-{
-	int ret = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&phy->lock, flags);
-	if (phy->enabled)
-		goto err1;
-
-	ret = ti_pipe3_enable_refclk(phy);
-	if (ret)
-		goto err1;
-
 	if (!IS_ERR(phy->wkupclk)) {
 		ret = clk_prepare_enable(phy->wkupclk);
 		if (ret) {
 			dev_err(phy->dev, "Failed to enable wkupclk %d\n", ret);
-			goto err2;
+			goto disable_refclk;
 		}
 	}
 
@@ -477,96 +451,33 @@ static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
 		ret = clk_prepare_enable(phy->div_clk);
 		if (ret) {
 			dev_err(phy->dev, "Failed to enable div_clk %d\n", ret);
-			goto err3;
+			goto disable_wkupclk;
 		}
 	}
 
-	phy->enabled = true;
-	spin_unlock_irqrestore(&phy->lock, flags);
 	return 0;
 
-err3:
+disable_wkupclk:
 	if (!IS_ERR(phy->wkupclk))
 		clk_disable_unprepare(phy->wkupclk);
 
-err2:
+disable_refclk:
 	if (!IS_ERR(phy->refclk))
 		clk_disable_unprepare(phy->refclk);
 
-	ti_pipe3_disable_refclk(phy);
-err1:
-	spin_unlock_irqrestore(&phy->lock, flags);
 	return ret;
 }
 
 static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&phy->lock, flags);
-	if (!phy->enabled) {
-		spin_unlock_irqrestore(&phy->lock, flags);
-		return;
-	}
-
 	if (!IS_ERR(phy->wkupclk))
 		clk_disable_unprepare(phy->wkupclk);
-	/* Don't disable refclk for SATA PHY due to Errata i783 */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata"))
-		ti_pipe3_disable_refclk(phy);
+	if (!IS_ERR(phy->refclk))
+		clk_disable_unprepare(phy->refclk);
 	if (!IS_ERR(phy->div_clk))
 		clk_disable_unprepare(phy->div_clk);
-	phy->enabled = false;
-	spin_unlock_irqrestore(&phy->lock, flags);
-}
-
-static int ti_pipe3_runtime_suspend(struct device *dev)
-{
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-
-	ti_pipe3_disable_clocks(phy);
-	return 0;
 }
 
-static int ti_pipe3_runtime_resume(struct device *dev)
-{
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-	int ret = 0;
-
-	ret = ti_pipe3_enable_clocks(phy);
-	return ret;
-}
-
-static int ti_pipe3_suspend(struct device *dev)
-{
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-
-	ti_pipe3_disable_clocks(phy);
-	return 0;
-}
-
-static int ti_pipe3_resume(struct device *dev)
-{
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-	int ret;
-
-	ret = ti_pipe3_enable_clocks(phy);
-	if (ret)
-		return ret;
-
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-	return 0;
-}
-#endif
-
-static const struct dev_pm_ops ti_pipe3_pm_ops = {
-	SET_RUNTIME_PM_OPS(ti_pipe3_runtime_suspend,
-			   ti_pipe3_runtime_resume, NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(ti_pipe3_suspend, ti_pipe3_resume)
-};
-
 static const struct of_device_id ti_pipe3_id_table[] = {
 	{
 		.compatible = "ti,phy-usb3",
@@ -592,7 +503,6 @@ static struct platform_driver ti_pipe3_driver = {
 	.remove		= ti_pipe3_remove,
 	.driver		= {
 		.name	= "ti-pipe3",
-		.pm	= &ti_pipe3_pm_ops,
 		.of_match_table = ti_pipe3_id_table,
 	},
 };
-- 
2.1.4

[PATCH v2 2/3] phy: ti-pipe3: i783 workaround for SATA lockup after dpll unlock/relock

[Roger Quadros]

SATA_PLL_SOFT_RESET bit of CTRL_CORE_SMA_SW_0 must be toggled
between a SATA DPLL unlock and re-lock to prevent SATA lockup.

Introduce a new DT parameter 'syscon-pllreset' to provide the syscon
regmap access to this register which sits in the control module.

If the register is not provided we fallback to the old behaviour
i.e. SATA DPLL refclk will not be disabled and we prevent SoC low
power states.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 Documentation/devicetree/bindings/phy/ti-phy.txt | 16 +++++++
 drivers/phy/phy-ti-pipe3.c                       | 61 +++++++++++++++++++++---
 2 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
index 305e3df..f0f5537 100644
--- a/Documentation/devicetree/bindings/phy/ti-phy.txt
+++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
@@ -82,6 +82,9 @@ Optional properties:
  - id: If there are multiple instance of the same type, in order to
    differentiate between each instance "id" can be used (e.g., multi-lane PCIe
    PHY). If "id" is not provided, it is set to default value of '1'.
+ - syscon-pllreset: Handle to system control region that contains the
+   CTRL_CORE_SMA_SW_0 register and register offset to the CTRL_CORE_SMA_SW_0
+   register that contains the SATA_PLL_SOFT_RESET bit. Only valid for sata_phy.
 
 This is usually a subnode of ocp2scp to which it is connected.
 
@@ -100,3 +103,16 @@ usb3phy@4a084400 {
 			"sysclk",
 			"refclk";
 };
+
+sata_phy: phy@4A096000 {
+	compatible = "ti,phy-pipe3-sata";
+	reg = <0x4A096000 0x80>, /* phy_rx */
+	      <0x4A096400 0x64>, /* phy_tx */
+	      <0x4A096800 0x40>; /* pll_ctrl */
+	reg-names = "phy_rx", "phy_tx", "pll_ctrl";
+	ctrl-module = <&omap_control_sata>;
+	clocks = <&sys_clkin1>, <&sata_ref_clk>;
+	clock-names = "sysclk", "refclk";
+	syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
+	#phy-cells = <0>;
+};
diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
index 3510b81..08020dc 100644
--- a/drivers/phy/phy-ti-pipe3.c
+++ b/drivers/phy/phy-ti-pipe3.c
@@ -28,6 +28,8 @@
 #include <linux/delay.h>
 #include <linux/phy/omap_control_phy.h>
 #include <linux/of_platform.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 #define	PLL_STATUS		0x00000004
 #define	PLL_GO			0x00000008
@@ -52,6 +54,8 @@
 #define	PLL_LOCK		0x2
 #define	PLL_IDLE		0x1
 
+#define SATA_PLL_SOFT_RESET	BIT(18)
+
 /*
  * This is an Empirical value that works, need to confirm the actual
  * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status
@@ -82,6 +86,9 @@ struct ti_pipe3 {
 	struct clk		*refclk;
 	struct clk		*div_clk;
 	struct pipe3_dpll_map	*dpll_map;
+	struct regmap		*dpll_reset_syscon; /* ctrl. reg. acces */
+	unsigned int		dpll_reset_reg; /* reg. index within syscon */
+	bool			sata_refclk_enabled;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {
@@ -249,8 +256,11 @@ static int ti_pipe3_exit(struct phy *x)
 	u32 val;
 	unsigned long timeout;
 
-	/* SATA DPLL can't be powered down due to Errata i783 */
-	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata"))
+	/* If dpll_reset_syscon is not present we wont power down SATA DPLL
+	 * due to Errata i783
+	 */
+	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata") &&
+	    !phy->dpll_reset_syscon)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
@@ -276,6 +286,14 @@ static int ti_pipe3_exit(struct phy *x)
 		}
 	}
 
+	/* i783: SATA needs control bit toggle after PLL unlock */
+	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata")) {
+		regmap_update_bits(phy->dpll_reset_syscon, phy->dpll_reset_reg,
+				   SATA_PLL_SOFT_RESET, SATA_PLL_SOFT_RESET);
+		regmap_update_bits(phy->dpll_reset_syscon, phy->dpll_reset_reg,
+				   SATA_PLL_SOFT_RESET, 0);
+	}
+
 	ti_pipe3_disable_clocks(phy);
 
 	return 0;
@@ -350,6 +368,21 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 		}
 	} else {
 		phy->wkupclk = ERR_PTR(-ENODEV);
+		phy->dpll_reset_syscon = syscon_regmap_lookup_by_phandle(node,
+							"syscon-pllreset");
+		if (IS_ERR(phy->dpll_reset_syscon)) {
+			dev_info(&pdev->dev,
+				 "can't get syscon-pllreset, sata dpll won't idle\n");
+			phy->dpll_reset_syscon = NULL;
+		} else {
+			if (of_property_read_u32_index(node,
+						       "syscon-pllreset", 1,
+						       &phy->dpll_reset_reg)) {
+				dev_err(&pdev->dev,
+					"couldn't get pllreset reg. offset\n");
+				return -EINVAL;
+			}
+		}
 	}
 
 	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
@@ -402,10 +435,16 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, phy);
 	pm_runtime_enable(phy->dev);
-	/* Prevent auto-disable of refclk for SATA PHY due to Errata i783 */
-	if (of_device_is_compatible(node, "ti,phy-pipe3-sata"))
-		if (!IS_ERR(phy->refclk))
+
+	/*
+	 * Prevent auto-disable of refclk for SATA PHY due to Errata i783
+	 */
+	if (of_device_is_compatible(node, "ti,phy-pipe3-sata")) {
+		if (!IS_ERR(phy->refclk)) {
 			clk_prepare_enable(phy->refclk);
+			phy->sata_refclk_enabled = true;
+		}
+	}
 
 	generic_phy = devm_phy_create(phy->dev, NULL, &ops);
 	if (IS_ERR(generic_phy))
@@ -472,8 +511,18 @@ static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy)
 {
 	if (!IS_ERR(phy->wkupclk))
 		clk_disable_unprepare(phy->wkupclk);
-	if (!IS_ERR(phy->refclk))
+	if (!IS_ERR(phy->refclk)) {
 		clk_disable_unprepare(phy->refclk);
+		/*
+		 * SATA refclk needs an additional disable as we left it
+		 * on in probe to avoid Errata i783
+		 */
+		if (phy->sata_refclk_enabled) {
+			clk_disable_unprepare(phy->refclk);
+			phy->sata_refclk_enabled = false;
+		}
+	}
+
 	if (!IS_ERR(phy->div_clk))
 		clk_disable_unprepare(phy->div_clk);
 }
-- 
2.1.4

[PATCH v2 2/3] phy: ti-pipe3: i783 workaround for SATA lockup after dpll unlock/relock

[Roger Quadros]

SATA_PLL_SOFT_RESET bit of CTRL_CORE_SMA_SW_0 must be toggled
between a SATA DPLL unlock and re-lock to prevent SATA lockup.

Introduce a new DT parameter 'syscon-pllreset' to provide the syscon
regmap access to this register which sits in the control module.

If the register is not provided we fallback to the old behaviour
i.e. SATA DPLL refclk will not be disabled and we prevent SoC low
power states.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 Documentation/devicetree/bindings/phy/ti-phy.txt | 16 +++++++
 drivers/phy/phy-ti-pipe3.c                       | 61 +++++++++++++++++++++---
 2 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
index 305e3df..f0f5537 100644
--- a/Documentation/devicetree/bindings/phy/ti-phy.txt
+++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
@@ -82,6 +82,9 @@ Optional properties:
  - id: If there are multiple instance of the same type, in order to
    differentiate between each instance "id" can be used (e.g., multi-lane PCIe
    PHY). If "id" is not provided, it is set to default value of '1'.
+ - syscon-pllreset: Handle to system control region that contains the
+   CTRL_CORE_SMA_SW_0 register and register offset to the CTRL_CORE_SMA_SW_0
+   register that contains the SATA_PLL_SOFT_RESET bit. Only valid for sata_phy.
 
 This is usually a subnode of ocp2scp to which it is connected.
 
@@ -100,3 +103,16 @@ usb3phy@4a084400 {
 			"sysclk",
 			"refclk";
 };
+
+sata_phy: phy@4A096000 {
+	compatible = "ti,phy-pipe3-sata";
+	reg = <0x4A096000 0x80>, /* phy_rx */
+	      <0x4A096400 0x64>, /* phy_tx */
+	      <0x4A096800 0x40>; /* pll_ctrl */
+	reg-names = "phy_rx", "phy_tx", "pll_ctrl";
+	ctrl-module = <&omap_control_sata>;
+	clocks = <&sys_clkin1>, <&sata_ref_clk>;
+	clock-names = "sysclk", "refclk";
+	syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
+	#phy-cells = <0>;
+};
diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
index 3510b81..08020dc 100644
--- a/drivers/phy/phy-ti-pipe3.c
+++ b/drivers/phy/phy-ti-pipe3.c
@@ -28,6 +28,8 @@
 #include <linux/delay.h>
 #include <linux/phy/omap_control_phy.h>
 #include <linux/of_platform.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 #define	PLL_STATUS		0x00000004
 #define	PLL_GO			0x00000008
@@ -52,6 +54,8 @@
 #define	PLL_LOCK		0x2
 #define	PLL_IDLE		0x1
 
+#define SATA_PLL_SOFT_RESET	BIT(18)
+
 /*
  * This is an Empirical value that works, need to confirm the actual
  * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status
@@ -82,6 +86,9 @@ struct ti_pipe3 {
 	struct clk		*refclk;
 	struct clk		*div_clk;
 	struct pipe3_dpll_map	*dpll_map;
+	struct regmap		*dpll_reset_syscon; /* ctrl. reg. acces */
+	unsigned int		dpll_reset_reg; /* reg. index within syscon */
+	bool			sata_refclk_enabled;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {
@@ -249,8 +256,11 @@ static int ti_pipe3_exit(struct phy *x)
 	u32 val;
 	unsigned long timeout;
 
-	/* SATA DPLL can't be powered down due to Errata i783 */
-	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata"))
+	/* If dpll_reset_syscon is not present we wont power down SATA DPLL
+	 * due to Errata i783
+	 */
+	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata") &&
+	    !phy->dpll_reset_syscon)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
@@ -276,6 +286,14 @@ static int ti_pipe3_exit(struct phy *x)
 		}
 	}
 
+	/* i783: SATA needs control bit toggle after PLL unlock */
+	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata")) {
+		regmap_update_bits(phy->dpll_reset_syscon, phy->dpll_reset_reg,
+				   SATA_PLL_SOFT_RESET, SATA_PLL_SOFT_RESET);
+		regmap_update_bits(phy->dpll_reset_syscon, phy->dpll_reset_reg,
+				   SATA_PLL_SOFT_RESET, 0);
+	}
+
 	ti_pipe3_disable_clocks(phy);
 
 	return 0;
@@ -350,6 +368,21 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 		}
 	} else {
 		phy->wkupclk = ERR_PTR(-ENODEV);
+		phy->dpll_reset_syscon = syscon_regmap_lookup_by_phandle(node,
+							"syscon-pllreset");
+		if (IS_ERR(phy->dpll_reset_syscon)) {
+			dev_info(&pdev->dev,
+				 "can't get syscon-pllreset, sata dpll won't idle\n");
+			phy->dpll_reset_syscon = NULL;
+		} else {
+			if (of_property_read_u32_index(node,
+						       "syscon-pllreset", 1,
+						       &phy->dpll_reset_reg)) {
+				dev_err(&pdev->dev,
+					"couldn't get pllreset reg. offset\n");
+				return -EINVAL;
+			}
+		}
 	}
 
 	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
@@ -402,10 +435,16 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, phy);
 	pm_runtime_enable(phy->dev);
-	/* Prevent auto-disable of refclk for SATA PHY due to Errata i783 */
-	if (of_device_is_compatible(node, "ti,phy-pipe3-sata"))
-		if (!IS_ERR(phy->refclk))
+
+	/*
+	 * Prevent auto-disable of refclk for SATA PHY due to Errata i783
+	 */
+	if (of_device_is_compatible(node, "ti,phy-pipe3-sata")) {
+		if (!IS_ERR(phy->refclk)) {
 			clk_prepare_enable(phy->refclk);
+			phy->sata_refclk_enabled = true;
+		}
+	}
 
 	generic_phy = devm_phy_create(phy->dev, NULL, &ops);
 	if (IS_ERR(generic_phy))
@@ -472,8 +511,18 @@ static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy)
 {
 	if (!IS_ERR(phy->wkupclk))
 		clk_disable_unprepare(phy->wkupclk);
-	if (!IS_ERR(phy->refclk))
+	if (!IS_ERR(phy->refclk)) {
 		clk_disable_unprepare(phy->refclk);
+		/*
+		 * SATA refclk needs an additional disable as we left it
+		 * on in probe to avoid Errata i783
+		 */
+		if (phy->sata_refclk_enabled) {
+			clk_disable_unprepare(phy->refclk);
+			phy->sata_refclk_enabled = false;
+		}
+	}
+
 	if (!IS_ERR(phy->div_clk))
 		clk_disable_unprepare(phy->div_clk);
 }
-- 
2.1.4

[PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Roger Quadros]

This register is required to be passed to the SATA PHY driver
to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index f03a091..260f300 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1135,6 +1135,7 @@
 				ctrl-module = <&omap_control_sata>;
 				clocks = <&sys_clkin1>, <&sata_ref_clk>;
 				clock-names = "sysclk", "refclk";
+				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
 				#phy-cells = <0>;
 			};
 
-- 
2.1.4

[PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Roger Quadros]

This register is required to be passed to the SATA PHY driver
to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index f03a091..260f300 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1135,6 +1135,7 @@
 				ctrl-module = <&omap_control_sata>;
 				clocks = <&sys_clkin1>, <&sata_ref_clk>;
 				clock-names = "sysclk", "refclk";
+				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
 				#phy-cells = <0>;
 			};
 
-- 
2.1.4

Re: [PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Kishon Vijay Abraham I]

Hi Roger,

On Tuesday 02 June 2015 02:40 PM, Roger Quadros wrote:
> This register is required to be passed to the SATA PHY driver
> to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> ---
>  arch/arm/boot/dts/dra7.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index f03a091..260f300 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -1135,6 +1135,7 @@
>  				ctrl-module = <&omap_control_sata>;
>  				clocks = <&sys_clkin1>, <&sata_ref_clk>;
>  				clock-names = "sysclk", "refclk";
> +				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;

I think all users of syscon should be made child node of scm_conf. Tony and
Tero, is that right?

If so, then we might have to modify the driver too.

Cheers
Kishon

Re: [PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Kishon Vijay Abraham I]

Hi Roger,

On Tuesday 02 June 2015 02:40 PM, Roger Quadros wrote:
> This register is required to be passed to the SATA PHY driver
> to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> ---
>  arch/arm/boot/dts/dra7.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index f03a091..260f300 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -1135,6 +1135,7 @@
>  				ctrl-module = <&omap_control_sata>;
>  				clocks = <&sys_clkin1>, <&sata_ref_clk>;
>  				clock-names = "sysclk", "refclk";
> +				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;

I think all users of syscon should be made child node of scm_conf. Tony and
Tero, is that right?

If so, then we might have to modify the driver too.

Cheers
Kishon

Re: [PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Tony Lindgren]

* Kishon Vijay Abraham I <kishon@ti.com> [150715 04:24]:
> Hi Roger,
> 
> On Tuesday 02 June 2015 02:40 PM, Roger Quadros wrote:
> > This register is required to be passed to the SATA PHY driver
> > to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).
> > 
> > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> > ---
> >  arch/arm/boot/dts/dra7.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> > index f03a091..260f300 100644
> > --- a/arch/arm/boot/dts/dra7.dtsi
> > +++ b/arch/arm/boot/dts/dra7.dtsi
> > @@ -1135,6 +1135,7 @@
> >  				ctrl-module = <&omap_control_sata>;
> >  				clocks = <&sys_clkin1>, <&sata_ref_clk>;
> >  				clock-names = "sysclk", "refclk";
> > +				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
> 
> I think all users of syscon should be made child node of scm_conf. Tony and
> Tero, is that right?
> 
> If so, then we might have to modify the driver too.

Yeah there should not be much need to use syscon outside scm_conf
area and for I2C devices. If there's some other misc register area
in dra7 in addition to scm_conf then it might make sense to use it.

But in general, for the SCM registers, just a normal loadable kernel
driver module doing of_ioremap on a dedicated range of registers is
always a better option :)

Regrds,

Tony

Re: [PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Roger Quadros]

Hi,

On 15/07/15 15:07, Tony Lindgren wrote:
> * Kishon Vijay Abraham I <kishon@ti.com> [150715 04:24]:
>> Hi Roger,
>>
>> On Tuesday 02 June 2015 02:40 PM, Roger Quadros wrote:
>>> This register is required to be passed to the SATA PHY driver
>>> to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>> ---
>>>  arch/arm/boot/dts/dra7.dtsi | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>>> index f03a091..260f300 100644
>>> --- a/arch/arm/boot/dts/dra7.dtsi
>>> +++ b/arch/arm/boot/dts/dra7.dtsi
>>> @@ -1135,6 +1135,7 @@
>>>  				ctrl-module = <&omap_control_sata>;
>>>  				clocks = <&sys_clkin1>, <&sata_ref_clk>;
>>>  				clock-names = "sysclk", "refclk";
>>> +				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
>>
>> I think all users of syscon should be made child node of scm_conf. Tony and
>> Tero, is that right?

It can't be child of scm_conf as the address is outside it's range.
Looks like I have to add a new child to scm node that maps beyond
the dra7_pmx_core padconf address range.

>>
>> If so, then we might have to modify the driver too.
> 
> Yeah there should not be much need to use syscon outside scm_conf
> area and for I2C devices. If there's some other misc register area
> in dra7 in addition to scm_conf then it might make sense to use it.
> 
> But in general, for the SCM registers, just a normal loadable kernel
> driver module doing of_ioremap on a dedicated range of registers is
> always a better option :)
> 

Lets take for example this register CTRL_CORE_SMA_SW_0.
It has the SATA PLL_SOFT_RESET bit, EMIF1/2 gating control bits
and ISOLATE bit.

I don't see this fitting in any driver except the syscon approach.

cheers,
-roger

Re: [PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Roger Quadros]

Hi,

On 15/07/15 15:07, Tony Lindgren wrote:
> * Kishon Vijay Abraham I <kishon@ti.com> [150715 04:24]:
>> Hi Roger,
>>
>> On Tuesday 02 June 2015 02:40 PM, Roger Quadros wrote:
>>> This register is required to be passed to the SATA PHY driver
>>> to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>> ---
>>>  arch/arm/boot/dts/dra7.dtsi | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>>> index f03a091..260f300 100644
>>> --- a/arch/arm/boot/dts/dra7.dtsi
>>> +++ b/arch/arm/boot/dts/dra7.dtsi
>>> @@ -1135,6 +1135,7 @@
>>>  				ctrl-module = <&omap_control_sata>;
>>>  				clocks = <&sys_clkin1>, <&sata_ref_clk>;
>>>  				clock-names = "sysclk", "refclk";
>>> +				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
>>
>> I think all users of syscon should be made child node of scm_conf. Tony and
>> Tero, is that right?

It can't be child of scm_conf as the address is outside it's range.
Looks like I have to add a new child to scm node that maps beyond
the dra7_pmx_core padconf address range.

>>
>> If so, then we might have to modify the driver too.
> 
> Yeah there should not be much need to use syscon outside scm_conf
> area and for I2C devices. If there's some other misc register area
> in dra7 in addition to scm_conf then it might make sense to use it.
> 
> But in general, for the SCM registers, just a normal loadable kernel
> driver module doing of_ioremap on a dedicated range of registers is
> always a better option :)
> 

Lets take for example this register CTRL_CORE_SMA_SW_0.
It has the SATA PLL_SOFT_RESET bit, EMIF1/2 gating control bits
and ISOLATE bit.

I don't see this fitting in any driver except the syscon approach.

cheers,
-roger

Re: [PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Tero Kristo]

On 07/15/2015 04:47 PM, Roger Quadros wrote:
> Hi,
>
> On 15/07/15 15:07, Tony Lindgren wrote:
>> * Kishon Vijay Abraham I <kishon@ti.com> [150715 04:24]:
>>> Hi Roger,
>>>
>>> On Tuesday 02 June 2015 02:40 PM, Roger Quadros wrote:
>>>> This register is required to be passed to the SATA PHY driver
>>>> to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>>> ---
>>>>   arch/arm/boot/dts/dra7.dtsi | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>>>> index f03a091..260f300 100644
>>>> --- a/arch/arm/boot/dts/dra7.dtsi
>>>> +++ b/arch/arm/boot/dts/dra7.dtsi
>>>> @@ -1135,6 +1135,7 @@
>>>>   				ctrl-module = <&omap_control_sata>;
>>>>   				clocks = <&sys_clkin1>, <&sata_ref_clk>;
>>>>   				clock-names = "sysclk", "refclk";
>>>> +				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
>>>
>>> I think all users of syscon should be made child node of scm_conf. Tony and
>>> Tero, is that right?
>
> It can't be child of scm_conf as the address is outside it's range.
> Looks like I have to add a new child to scm node that maps beyond
> the dra7_pmx_core padconf address range.
>
>>>
>>> If so, then we might have to modify the driver too.
>>
>> Yeah there should not be much need to use syscon outside scm_conf
>> area and for I2C devices. If there's some other misc register area
>> in dra7 in addition to scm_conf then it might make sense to use it.
>>
>> But in general, for the SCM registers, just a normal loadable kernel
>> driver module doing of_ioremap on a dedicated range of registers is
>> always a better option :)
>>
>
> Lets take for example this register CTRL_CORE_SMA_SW_0.
> It has the SATA PLL_SOFT_RESET bit, EMIF1/2 gating control bits
> and ISOLATE bit.
>
> I don't see this fitting in any driver except the syscon approach.
>
> cheers,
> -roger
>

Yea I think scm_conf can generally contain lots of weird registers that 
can have multiple users / use-cases. This is the junk-yard of SoC 
features the designers had no idea where to put them; so they put it 
under scm_conf.

I'd say in some cases we are probably forced to map to it from other 
drivers, this is one of the reasons it is a syscon map in the first 
place, and generally speaking, they should not be children of scm_conf 
in the DT layout.

You could probably add a "dummy" node under scm_conf which maps to the 
scm register, and which you would refer to from the sata driver.

-Tero

Re: [PATCH v2 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY

[Tero Kristo]

On 07/15/2015 04:47 PM, Roger Quadros wrote:
> Hi,
>
> On 15/07/15 15:07, Tony Lindgren wrote:
>> * Kishon Vijay Abraham I <kishon@ti.com> [150715 04:24]:
>>> Hi Roger,
>>>
>>> On Tuesday 02 June 2015 02:40 PM, Roger Quadros wrote:
>>>> This register is required to be passed to the SATA PHY driver
>>>> to workaround errata i783 (SATA Lockup After SATA DPLL Unlock/Relock).
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>>> ---
>>>>   arch/arm/boot/dts/dra7.dtsi | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>>>> index f03a091..260f300 100644
>>>> --- a/arch/arm/boot/dts/dra7.dtsi
>>>> +++ b/arch/arm/boot/dts/dra7.dtsi
>>>> @@ -1135,6 +1135,7 @@
>>>>   				ctrl-module = <&omap_control_sata>;
>>>>   				clocks = <&sys_clkin1>, <&sata_ref_clk>;
>>>>   				clock-names = "sysclk", "refclk";
>>>> +				syscon-pllreset = <&dra7_ctrl_core 0x3fc>;
>>>
>>> I think all users of syscon should be made child node of scm_conf. Tony and
>>> Tero, is that right?
>
> It can't be child of scm_conf as the address is outside it's range.
> Looks like I have to add a new child to scm node that maps beyond
> the dra7_pmx_core padconf address range.
>
>>>
>>> If so, then we might have to modify the driver too.
>>
>> Yeah there should not be much need to use syscon outside scm_conf
>> area and for I2C devices. If there's some other misc register area
>> in dra7 in addition to scm_conf then it might make sense to use it.
>>
>> But in general, for the SCM registers, just a normal loadable kernel
>> driver module doing of_ioremap on a dedicated range of registers is
>> always a better option :)
>>
>
> Lets take for example this register CTRL_CORE_SMA_SW_0.
> It has the SATA PLL_SOFT_RESET bit, EMIF1/2 gating control bits
> and ISOLATE bit.
>
> I don't see this fitting in any driver except the syscon approach.
>
> cheers,
> -roger
>

Yea I think scm_conf can generally contain lots of weird registers that 
can have multiple users / use-cases. This is the junk-yard of SoC 
features the designers had no idea where to put them; so they put it 
under scm_conf.

I'd say in some cases we are probably forced to map to it from other 
drivers, this is one of the reasons it is a syscon map in the first 
place, and generally speaking, they should not be children of scm_conf 
in the DT layout.

You could probably add a "dummy" node under scm_conf which maps to the 
scm register, and which you would refer to from the sata driver.

-Tero