phy: ti-pipe3: fixes for 3.19-rc

On system suspend, the runtime_suspend() driver hook doesn't get called and so the clocks are not disabled in the driver. This causes the L3INIT_960M_GFCLK and L3INIT_480M_GFCLK to remain active on the DRA7 platform while in system suspend.

Add suspend/resume hooks to the driver.
In case of pcie-phy, the runtime_suspend hook gets called after

the suspend hook so we introduce a flag phy->enabled to keep track if our clocks are enabled or not to prevent multiple enable/disables.

Move enabling/disabling clock code into helper functions.

Reported-by Nishant Menon <[email protected]>

Signed-off-by Roger Quadros <[email protected]>

---
 drivers/phy/phy-ti-pipe3.c | 99 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 77 insertions(+), 22 deletions(-)

drivers/phy/phy-ti-pipe3.c +77 -22

index 1387b4d..e60ff14 100644
@@ -28,6 +28,7 @@
 #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,6 +84,8 @@ struct ti_pipe3 {
 	struct clk		*div_clk;
 	struct pipe3_dpll_map	*dpll_map;
 	u8			id;
+	bool enabled;
+	spinlock_t lock;	/* serialize clock enable/disable */
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -303,6 +306,7 @@ 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(of_match_ptr(ti_pipe3_id_table),

@@ -425,24 +429,14 @@ static int ti_pipe3_remove(struct platform_device *pdev)
 
 #ifdef CONFIG_PM
 
-static int ti_pipe3_runtime_suspend(struct device *dev)
+static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
-
-	if (!IS_ERR(phy->wkupclk))
-		clk_disable_unprepare(phy->wkupclk);
-	if (!IS_ERR(phy->refclk))
-		clk_disable_unprepare(phy->refclk);
-	if (!IS_ERR(phy->div_clk))
-		clk_disable_unprepare(phy->div_clk);
-
-	return 0;
-}
+	int ret = 0;
+	unsigned long flags;
 
-static int ti_pipe3_runtime_resume(struct device *dev)
-{
-	u32 ret = 0;
-	struct ti_pipe3	*phy = dev_get_drvdata(dev);
+	spin_lock_irqsave(&phy->lock, flags);
+	if (phy->enabled)
+		goto err1;
 
 	if (!IS_ERR(phy->refclk)) {
 		ret = clk_prepare_enable(phy->refclk);

@@ -467,6 +461,9 @@ static int ti_pipe3_runtime_resume(struct device *dev)
 			goto err3;
 		}
 	}
+
+	phy->enabled = true;
+	spin_unlock_irqrestore(&phy->lock, flags);
 	return 0;
 
 err3:

@@ -478,19 +475,77 @@ err2:
 		clk_disable_unprepare(phy->refclk);
 
 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);
+	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)
 };
 
-#define DEV_PM_OPS     (&ti_pipe3_pm_ops)
-#else
-#define DEV_PM_OPS     NULL
-#endif
-
 #ifdef CONFIG_OF
 static const struct of_device_id ti_pipe3_id_table[] = {
 	{

@@ -518,7 +573,7 @@ static struct platform_driver ti_pipe3_driver = {
 	.remove		= ti_pipe3_remove,
 	.driver		= {
 		.name	= "ti-pipe3",
-		.pm	= DEV_PM_OPS,
+		.pm	= &ti_pipe3_pm_ops,
 		.of_match_table = of_match_ptr(ti_pipe3_id_table),
 	},
 };

Hi Roger,

Quoted 5 lines · Roger Quadros

On system suspend, the runtime_suspend() driver hook doesn't get called and so the clocks are not disabled in the driver. This causes the L3INIT_960M_GFCLK and L3INIT_480M_GFCLK to remain active on the DRA7 platform while in system suspend.

Quoted 2 lines

Add suspend/resume hooks to the driver.
In case of pcie-phy, the runtime_suspend hook gets called after

This contradicts with the first line of your commit message. Is pcie-phy driver is an exception?

Thanks
Kishon

Quoted 8 lines

the suspend hook so we introduce a flag phy->enabled to keep track if our clocks are enabled or not to prevent multiple enable/disables.

Move enabling/disabling clock code into helper functions.

Reported-by Nishant Menon <[email protected]>

Signed-off-by Roger Quadros <[email protected]>

Failed test case: Boot without SATA drive connected. Suspend/resume the board and then connect SATA drive. It fails to enumerate.

Due to Errata i783 "SATA Lockup After SATA DPLL Unlock/Relock" we can't allow SATA DPLL to be in the unlocked state.
The SATA refclk (sata_ref_clk) is the source of the SATA_DPLL. Till now this clock was controlled by the AHCI SATA driver and was being shut off during system suspend (if the SATA drive was not already attached) causing the SATA DPLL to be unlocked and so causing errata i783.

To prevent sata_ref_clk from being disabled, we move the control of this clock from the SATA AHCI driver to the SATA PHY driver and prevent it from being disabled.

This also fixes the issue of SATA not working on OMAP5/DRA7 when AHCI platform driver is built as a module.

Signed-off-by Roger Quadros <[email protected]>

---
 arch/arm/boot/dts/dra7.dtsi  |  4 ++--
 arch/arm/boot/dts/omap5.dtsi |  4 ++--
 drivers/phy/phy-ti-pipe3.c   | 53 +++++++++++++++++++++++++++++++-------------
 3 files changed, 41 insertions(+), 20 deletions(-)

arch/arm/boot/dts/dra7.dtsi +2 -2

index 63bf99b..8c35b84 100644
@@ -1090,8 +1090,8 @@
 				      <0x4A096800 0x40>; /* pll_ctrl */
 				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
 				ctrl-module = <&omap_control_sata>;
-				clocks = <&sys_clkin1>;
-				clock-names = "sysclk";
+				clocks = <&sys_clkin1>, <&sata_ref_clk>;
+				clock-names = "sysclk", "refclk";
 				#phy-cells = <0>;
 			};
 

arch/arm/boot/dts/omap5.dtsi +2 -2

index b321fdf..bb498e7 100644
@@ -929,8 +929,8 @@
 				      <0x4A096800 0x40>; /* pll_ctrl */
 				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
 				ctrl-module = <&omap_control_sata>;
-				clocks = <&sys_clkin>;
-				clock-names = "sysclk";
+				clocks = <&sys_clkin>, <&sata_ref_clk>;
+				clock-names = "sysclk", "refclk";
 				#phy-cells = <0>;
 			};
 		};

drivers/phy/phy-ti-pipe3.c +1 -0

index e60ff14..e08edd9 100644
@@ -85,6 +85,7 @@ struct ti_pipe3 {
 	struct pipe3_dpll_map	*dpll_map;
 	u8			id;
 	bool enabled;
+	bool refclk_enabled;	/* this flag is needed specifically for SATA */
 	spinlock_t lock;	/* serialize clock enable/disable */
 };
 

Hi Roger,

Quoted 70 lines · Roger Quadros

Failed test case: Boot without SATA drive connected. Suspend/resume the board and then connect SATA drive. It fails to enumerate.

Due to Errata i783 "SATA Lockup After SATA DPLL Unlock/Relock" we can't allow SATA DPLL to be in the unlocked state.
The SATA refclk (sata_ref_clk) is the source of the SATA_DPLL. Till now this clock was controlled by the AHCI SATA driver and was being shut off during system suspend (if the SATA drive was not already attached) causing the SATA DPLL to be unlocked and so causing errata i783.

To prevent sata_ref_clk from being disabled, we move the control of this clock from the SATA AHCI driver to the SATA PHY driver and prevent it from being disabled.

This also fixes the issue of SATA not working on OMAP5/DRA7 when AHCI platform driver is built as a module.

Signed-off-by Roger Quadros <[email protected]>

---
 arch/arm/boot/dts/dra7.dtsi  |  4 ++--
 arch/arm/boot/dts/omap5.dtsi |  4 ++--
 drivers/phy/phy-ti-pipe3.c   | 53 +++++++++++++++++++++++++++++++-------------
 3 files changed, 41 insertions(+), 20 deletions(-)

arch/arm/boot/dts/dra7.dtsi +2 -2

index 63bf99b..8c35b84 100644
@@ -1090,8 +1090,8 @@
 				      <0x4A096800 0x40>; /* pll_ctrl */
 				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
 				ctrl-module = <&omap_control_sata>;
-				clocks = <&sys_clkin1>;
-				clock-names = "sysclk";
+				clocks = <&sys_clkin1>, <&sata_ref_clk>;
+				clock-names = "sysclk", "refclk";
 				#phy-cells = <0>;
 			};
 

arch/arm/boot/dts/omap5.dtsi +2 -2

index b321fdf..bb498e7 100644
@@ -929,8 +929,8 @@
 				      <0x4A096800 0x40>; /* pll_ctrl */
 				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
 				ctrl-module = <&omap_control_sata>;
-				clocks = <&sys_clkin>;
-				clock-names = "sysclk";
+				clocks = <&sys_clkin>, <&sata_ref_clk>;
+				clock-names = "sysclk", "refclk";
 				#phy-cells = <0>;
 			};
 		};

drivers/phy/phy-ti-pipe3.c +1 -0

index e60ff14..e08edd9 100644
@@ -85,6 +85,7 @@ struct ti_pipe3 {
 	struct pipe3_dpll_map	*dpll_map;
 	u8			id;
 	bool enabled;
+	bool refclk_enabled;	/* this flag is needed specifically for SATA */
 	spinlock_t lock;	/* serialize clock enable/disable */
 };
 

@@ -333,21 +334,20 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 		}
 	}
 

drivers/phy/phy-ti-pipe3.c static int ti_pipe3_probe(struct platform_device *pdev) · line 341

Quoted 5 lines

drivers/phy/phy-ti-pipe3.c +5 -0

+	phy->refclk = devm_clk_get(phy->dev, "refclk");
+	if (IS_ERR(phy->refclk)) {
+		dev_err(&pdev->dev, "unable to get refclk\n");
+		return PTR_ERR(phy->refclk);
+	}

This will break older dtbs. AFAIK, newer kernels should be compatible with older dtbs too. cc'ed [email protected] for clarification.

Failed test case: Boot without SATA drive connected. Suspend/resume the board and then connect SATA drive. It fails to enumerate.

Due to Errata i783 "SATA Lockup After SATA DPLL Unlock/Relock" we can't allow SATA DPLL to be in the unlocked state.
The SATA refclk (sata_ref_clk) is the source of the SATA_DPLL. Till now this clock was controlled by the AHCI SATA driver and was being shut off during system suspend (if the SATA drive was not already attached) causing the SATA DPLL to be unlocked and so causing errata i783.

To prevent sata_ref_clk from being disabled, we move the control of this clock from the SATA AHCI driver to the SATA PHY driver and prevent it from being disabled.

This also fixes the issue of SATA not working on OMAP5/DRA7 when AHCI platform driver is built as a module.

Signed-off-by Roger Quadros <[email protected]>

---
v2: don't bail out for missing refclk on SATA phy to work with older broken DTBs.

 arch/arm/boot/dts/dra7.dtsi  |  4 ++--
 arch/arm/boot/dts/omap5.dtsi |  4 ++--
 drivers/phy/phy-ti-pipe3.c   | 57 +++++++++++++++++++++++++++++++-------------
 3 files changed, 45 insertions(+), 20 deletions(-)

arch/arm/boot/dts/dra7.dtsi +2 -2

index 22771bc..8d2a635 100644
@@ -1090,8 +1090,8 @@
 				      <0x4A096800 0x40>; /* pll_ctrl */
 				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
 				ctrl-module = <&omap_control_sata>;
-				clocks = <&sys_clkin1>;
-				clock-names = "sysclk";
+				clocks = <&sys_clkin1>, <&sata_ref_clk>;
+				clock-names = "sysclk", "refclk";
 				#phy-cells = <0>;
 			};
 

arch/arm/boot/dts/omap5.dtsi +2 -2

index b321fdf..bb498e7 100644
@@ -929,8 +929,8 @@
 				      <0x4A096800 0x40>; /* pll_ctrl */
 				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
 				ctrl-module = <&omap_control_sata>;
-				clocks = <&sys_clkin>;
-				clock-names = "sysclk";
+				clocks = <&sys_clkin>, <&sata_ref_clk>;
+				clock-names = "sysclk", "refclk";
 				#phy-cells = <0>;
 			};
 		};

drivers/phy/phy-ti-pipe3.c +41 -16

index e60ff14..456dec2 100644
@@ -85,6 +85,7 @@ struct ti_pipe3 {
 	struct pipe3_dpll_map	*dpll_map;
 	u8			id;
 	bool enabled;
+	bool refclk_enabled;	/* this flag is needed specifically for SATA */
 	spinlock_t lock;	/* serialize clock enable/disable */
 };
 

@@ -333,21 +334,24 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 		}
 	}
 
+	phy->refclk = devm_clk_get(phy->dev, "refclk");
+	if (IS_ERR(phy->refclk)) {
+		dev_err(&pdev->dev, "unable to get refclk\n");
+		/* older DTBs have missing refclk in SATA PHY
+		 * so don't bail out in case of SATA PHY.
+		 */
+		if (!of_device_is_compatible(node, "ti,phy-pipe3-sata"))
+			return PTR_ERR(phy->refclk);
+	}
+
 	if (!of_device_is_compatible(node, "ti,phy-pipe3-sata")) {
 		phy->wkupclk = devm_clk_get(phy->dev, "wkupclk");
 		if (IS_ERR(phy->wkupclk)) {
 			dev_err(&pdev->dev, "unable to get wkupclk\n");
 			return PTR_ERR(phy->wkupclk);
 		}
-
-		phy->refclk = devm_clk_get(phy->dev, "refclk");
-		if (IS_ERR(phy->refclk)) {
-			dev_err(&pdev->dev, "unable to get refclk\n");
-			return PTR_ERR(phy->refclk);
-		}
 	} else {
 		phy->wkupclk = ERR_PTR(-ENODEV);
-		phy->refclk = ERR_PTR(-ENODEV);
 	}
 
 	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {

@@ -428,6 +432,29 @@ static int ti_pipe3_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
+static int ti_pipe3_enable_refclk(struct ti_pipe3 *phy)
+{
+	if (!IS_ERR(phy->refclk) && !phy->refclk_enabled) {
+		int ret;
+
+		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)
 {

@@ -438,13 +465,9 @@ static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
 	if (phy->enabled)
 		goto err1;
 
-	if (!IS_ERR(phy->refclk)) {
-		ret = clk_prepare_enable(phy->refclk);
-		if (ret) {
-			dev_err(phy->dev, "Failed to enable refclk %d\n", ret);
-			goto err1;
-		}
-	}
+	ret = ti_pipe3_enable_refclk(phy);
+	if (ret)
+		goto err1;
 
 	if (!IS_ERR(phy->wkupclk)) {
 		ret = clk_prepare_enable(phy->wkupclk);

@@ -474,6 +497,7 @@ err2:
 	if (!IS_ERR(phy->refclk))
 		clk_disable_unprepare(phy->refclk);
 
+	ti_pipe3_disable_refclk(phy);
 err1:
 	spin_unlock_irqrestore(&phy->lock, flags);
 	return ret;

@@ -491,8 +515,9 @@ 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))
-		clk_disable_unprepare(phy->refclk);
+	/* 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->div_clk))
 		clk_disable_unprepare(phy->div_clk);
 	phy->enabled = false;

Hi Roger,

Quoted 14 lines · Roger Quadros

Failed test case: Boot without SATA drive connected. Suspend/resume the board and then connect SATA drive. It fails to enumerate.

Due to Errata i783 "SATA Lockup After SATA DPLL Unlock/Relock" we can't allow SATA DPLL to be in the unlocked state.
The SATA refclk (sata_ref_clk) is the source of the SATA_DPLL. Till now this clock was controlled by the AHCI SATA driver and was being shut off during system suspend (if the SATA drive was not already attached) causing the SATA DPLL to be unlocked and so causing errata i783.

To prevent sata_ref_clk from being disabled, we move the control of this clock from the SATA AHCI driver to the SATA PHY driver and prevent it from being disabled.

Quoted 2 lines

This also fixes the issue of SATA not working on OMAP5/DRA7 when AHCI platform driver is built as a module.

I feel the dt patches and the PHY patches can go separately. Can you split and re-send?