Hello @Vinod Koul,

Thanks for review comments.
Please find my inline comments below with tag [Piyush].

Regards,
Piyush Mehta 

-----Original Message-----
From: Vinod Koul <vkoul@kernel.org>
Sent: Tuesday, September 20, 2022 12:11 PM
To: Mehta, Piyush <redacted>
Cc: laurent.pinchart@ideasonboard.com; kishon@ti.com;
michal.simek@xilinx.com; Simek, Michal [off-list ref]; linux-
kernel@vger.kernel.org; linux-phy@lists.infradead.org; linux-arm-
kernel@lists.infradead.org; git (AMD-Xilinx) [off-list ref]; Paladugu, Siva
Durga Prasad [off-list ref]
Subject: Re: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for
power-save

On 09-09-22, 18:34, Piyush Mehta wrote:

quoted

Enabling clock for all the lanes, consuming power even PHY is active
or not.

Pls consider revision to:
Enabling clock for all the lanes consumes power even PHY is active or idle..

[Piyush]:  will address in next version of patch.

quoted

To resolve the above issue and power saving,made clock
enabled/disabled based on active PHYs on call of phy_init/phy_exit.

To resolve this, enable/disable clocks in phy_init/phy_exit

[Piyush]:  will address in next version of patch.

quoted

By default clock is disabled for all the lanes. Whenever phy_init
called from USB, SATA, SGMII, or display driver, it enabled the
required clock for requested lane. On phy_exit cycle, it disabled
clock for the active PHYs.

During the suspend/resume cycle, each USB/ SATA/ SGMII/ display driver
called phy_exit/phy_init individually. it disabled clock on exit, and
enabled on initialization for the active PHYs.

Active PHY configuration depends on the peripheral DT node status,
like USB DT node status 'okay' then driver enabled clock for the USB
configured lane.

Signed-off-by: Piyush Mehta <redacted>
---
 drivers/phy/xilinx/phy-zynqmp.c | 59
++++++++-------------------------
 1 file changed, 14 insertions(+), 45 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c

b/drivers/phy/xilinx/phy-zynqmp.c index 9be9535ad7ab..912c4defdf8d
100644

--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c

@@ -572,6 +572,10 @@ static int xpsgtr_phy_init(struct phy *phy)

 	mutex_lock(&gtr_dev->gtr_mutex);

+	/* Configure and enable the clock when peripheral phy_init call */

when peripheral invokes phy_init

quoted

+	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->lane]))
+		goto out;
+
 	/* Skip initialization if not required. */
 	if (!xpsgtr_phy_init_required(gtr_phy))
 		goto out;

@@ -616,9 +620,13 @@ static int xpsgtr_phy_init(struct phy *phy)

static int xpsgtr_phy_exit(struct phy *phy)  {
 	struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
+	struct xpsgtr_dev *gtr_dev = gtr_phy->dev;

 	gtr_phy->skip_phy_init = false;

+	/* Ensure that disable clock only, which configure for lane */
+	clk_disable_unprepare(gtr_dev->clk[gtr_phy->lane]);
+
 	return 0;
 }

@@ -824,15 +832,11 @@ static struct phy *xpsgtr_xlate(struct device

*dev,  static int __maybe_unused xpsgtr_suspend(struct device *dev)  {
 	struct xpsgtr_dev *gtr_dev = dev_get_drvdata(dev);
-	unsigned int i;

 	/* Save the snapshot ICM_CFG registers. */
 	gtr_dev->saved_icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0);
 	gtr_dev->saved_icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1);

-	for (i = 0; i < ARRAY_SIZE(gtr_dev->clk); i++)
-		clk_disable_unprepare(gtr_dev->clk[i]);
-
 	return 0;
 }

@@ -842,13 +846,6 @@ static int __maybe_unused xpsgtr_resume(struct

device *dev)

quoted

 	unsigned int icm_cfg0, icm_cfg1;
 	unsigned int i;
 	bool skip_phy_init;
-	int err;
-
-	for (i = 0; i < ARRAY_SIZE(gtr_dev->clk); i++) {
-		err = clk_prepare_enable(gtr_dev->clk[i]);
-		if (err)
-			goto err_clk_put;
-	}

hold on, why is this being removed from suspend/resume?

[Piyush]:
Now, each peripheral driver calls phy_exit/phy_init in suspend/resume, and respective active driver will do clock enable and disable.

In above xpsgtr resume calls, by default all the clock enabled for all the lanes, that consumes power even PHY is active or not.
So, the clock will be enabled only for active PHY in the phy_init and disabled in phy_exit. Clock enabling is not required at multiple times.

quoted

 	icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0);
 	icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1); @@ -869,12 +866,6

@@

quoted

static int __maybe_unused xpsgtr_resume(struct device *dev)
 		gtr_dev->phys[i].skip_phy_init = skip_phy_init;

 	return 0;
-
-err_clk_put:
-	while (i--)
-		clk_disable_unprepare(gtr_dev->clk[i]);
-
-	return err;
 }

 static const struct dev_pm_ops xpsgtr_pm_ops = { @@ -888,7 +879,6 @@
static const struct dev_pm_ops xpsgtr_pm_ops = {  static int
xpsgtr_get_ref_clocks(struct xpsgtr_dev *gtr_dev)  {
 	unsigned int refclk;
-	int ret;

 	for (refclk = 0; refclk < ARRAY_SIZE(gtr_dev->refclk_sscs); ++refclk) {
 		unsigned long rate;

@@ -899,19 +889,14 @@ static int xpsgtr_get_ref_clocks(struct xpsgtr_dev

*gtr_dev)

quoted

 		snprintf(name, sizeof(name), "ref%u", refclk);
 		clk = devm_clk_get_optional(gtr_dev->dev, name);
 		if (IS_ERR(clk)) {
-			ret = dev_err_probe(gtr_dev->dev, PTR_ERR(clk),
-					    "Failed to get reference clock

%u\n",

quoted

-					    refclk);
-			goto err_clk_put;
+			return dev_err_probe(gtr_dev->dev, PTR_ERR(clk),
+					     "Failed to get ref clock %u\n",
+					     refclk);
 		}

 		if (!clk)
 			continue;

-		ret = clk_prepare_enable(clk);
-		if (ret)
-			goto err_clk_put;
-
 		gtr_dev->clk[refclk] = clk;

 		/*

@@ -931,18 +916,11 @@ static int xpsgtr_get_ref_clocks(struct xpsgtr_dev

*gtr_dev)

quoted

 			dev_err(gtr_dev->dev,
 				"Invalid rate %lu for reference clock %u\n",
 				rate, refclk);
-			ret = -EINVAL;
-			goto err_clk_put;
+			return -EINVAL;
 		}
 	}

 	return 0;
-
-err_clk_put:
-	while (refclk--)
-		clk_disable_unprepare(gtr_dev->clk[refclk]);
-
-	return ret;
 }

 static int xpsgtr_probe(struct platform_device *pdev) @@ -951,7
+929,6 @@ static int xpsgtr_probe(struct platform_device *pdev)
 	struct xpsgtr_dev *gtr_dev;
 	struct phy_provider *provider;
 	unsigned int port;
-	unsigned int i;
 	int ret;

 	gtr_dev = devm_kzalloc(&pdev->dev, sizeof(*gtr_dev), GFP_KERNEL);

@@

quoted

-991,8 +968,7 @@ static int xpsgtr_probe(struct platform_device *pdev)
 		phy = devm_phy_create(&pdev->dev, np, &xpsgtr_phyops);
 		if (IS_ERR(phy)) {
 			dev_err(&pdev->dev, "failed to create PHY\n");
-			ret = PTR_ERR(phy);
-			goto err_clk_put;
+			return PTR_ERR(phy);
 		}

 		gtr_phy->phy = phy;

@@ -1003,16 +979,9 @@ static int xpsgtr_probe(struct platform_device

*pdev)

quoted

 	provider = devm_of_phy_provider_register(&pdev->dev,

xpsgtr_xlate);

quoted

 	if (IS_ERR(provider)) {
 		dev_err(&pdev->dev, "registering provider failed\n");
-		ret = PTR_ERR(provider);
-		goto err_clk_put;
+		return PTR_ERR(provider);
 	}
 	return 0;
-
-err_clk_put:
-	for (i = 0; i < ARRAY_SIZE(gtr_dev->clk); i++)
-		clk_disable_unprepare(gtr_dev->clk[i]);
-
-	return ret;
 }

 static const struct of_device_id xpsgtr_of_match[] = {
--
2.25.1

--
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel