* [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save @ 2022-09-09 13:04 Piyush Mehta 2022-09-12 5:59 ` Michal Simek 2022-09-20 6:41 ` Vinod Koul 0 siblings, 2 replies; 6+ messages in thread From: Piyush Mehta @ 2022-09-09 13:04 UTC (permalink / raw) To: laurent.pinchart, kishon, vkoul, michal.simek Cc: michal.simek, linux-kernel, linux-phy, linux-arm-kernel, git, siva.durga.prasad.paladugu, Piyush Mehta Enabling clock for all the lanes, consuming power even PHY is active or not. To resolve the above issue and power saving,made clock enabled/disabled based on active PHYs on call of phy_init/phy_exit. 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 <piyush.mehta@amd.com> --- 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(>r_dev->gtr_mutex); + /* Configure and enable the clock when peripheral phy_init call */ + 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) 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; - } icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0); icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1); @@ -869,12 +866,6 @@ 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) 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", - 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) 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); @@ -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) provider = devm_of_phy_provider_register(&pdev->dev, xpsgtr_xlate); 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save 2022-09-09 13:04 [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save Piyush Mehta @ 2022-09-12 5:59 ` Michal Simek 2022-09-20 6:41 ` Vinod Koul 1 sibling, 0 replies; 6+ messages in thread From: Michal Simek @ 2022-09-12 5:59 UTC (permalink / raw) To: Piyush Mehta, laurent.pinchart, kishon, vkoul, michal.simek Cc: linux-kernel, linux-phy, linux-arm-kernel, git, siva.durga.prasad.paladugu On 9/9/22 15:04, Piyush Mehta wrote: > Enabling clock for all the lanes, consuming power even PHY is active or > not. > > To resolve the above issue and power saving,made clock enabled/disabled > based on active PHYs on call of phy_init/phy_exit. > > 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 <piyush.mehta@amd.com> > --- > 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(>r_dev->gtr_mutex); > > + /* Configure and enable the clock when peripheral phy_init call */ > + 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) > 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; > - } > > icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0); > icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1); > @@ -869,12 +866,6 @@ 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) > 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", > - 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) > 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); > @@ -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) > provider = devm_of_phy_provider_register(&pdev->dev, xpsgtr_xlate); > 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[] = { Acked-by: Michal Simek <michal.simek@amd.com> Thanks, Michal _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save 2022-09-09 13:04 [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save Piyush Mehta 2022-09-12 5:59 ` Michal Simek @ 2022-09-20 6:41 ` Vinod Koul 2022-09-20 10:38 ` Mehta, Piyush 1 sibling, 1 reply; 6+ messages in thread From: Vinod Koul @ 2022-09-20 6:41 UTC (permalink / raw) To: Piyush Mehta Cc: laurent.pinchart, kishon, michal.simek, michal.simek, linux-kernel, linux-phy, linux-arm-kernel, git, siva.durga.prasad.paladugu On 09-09-22, 18:34, Piyush Mehta wrote: > 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.. > > 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 > 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 <piyush.mehta@amd.com> > --- > 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(>r_dev->gtr_mutex); > > + /* Configure and enable the clock when peripheral phy_init call */ when peripheral invokes phy_init > + 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) > 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? > > icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0); > icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1); > @@ -869,12 +866,6 @@ 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) > 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", > - 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) > 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); > @@ -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) > provider = devm_of_phy_provider_register(&pdev->dev, xpsgtr_xlate); > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save 2022-09-20 6:41 ` Vinod Koul @ 2022-09-20 10:38 ` Mehta, Piyush 2022-09-20 11:49 ` Vinod Koul 0 siblings, 1 reply; 6+ messages in thread From: Mehta, Piyush @ 2022-09-20 10:38 UTC (permalink / raw) To: Vinod Koul Cc: laurent.pinchart, kishon, michal.simek, Simek, Michal, linux-kernel, linux-phy, linux-arm-kernel, git (AMD-Xilinx), Paladugu, Siva Durga Prasad 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 <piyush.mehta@amd.com> > Cc: laurent.pinchart@ideasonboard.com; kishon@ti.com; > michal.simek@xilinx.com; Simek, Michal <michal.simek@amd.com>; linux- > kernel@vger.kernel.org; linux-phy@lists.infradead.org; linux-arm- > kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; Paladugu, Siva > Durga Prasad <siva.durga.prasad.paladugu@amd.com> > Subject: Re: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for > power-save > > On 09-09-22, 18:34, Piyush Mehta wrote: > > 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. > > > > > 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. > > > 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 <piyush.mehta@amd.com> > > --- > > 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(>r_dev->gtr_mutex); > > > > + /* Configure and enable the clock when peripheral phy_init call */ > > when peripheral invokes phy_init > > > + 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) > > 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. > > > > > icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0); > > icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1); @@ -869,12 +866,6 > @@ > > 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) > > 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", > > - 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) > > 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); > @@ > > -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) > > provider = devm_of_phy_provider_register(&pdev->dev, > xpsgtr_xlate); > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save 2022-09-20 10:38 ` Mehta, Piyush @ 2022-09-20 11:49 ` Vinod Koul 2022-12-07 9:21 ` Mehta, Piyush 0 siblings, 1 reply; 6+ messages in thread From: Vinod Koul @ 2022-09-20 11:49 UTC (permalink / raw) To: Mehta, Piyush Cc: laurent.pinchart, kishon, michal.simek, Simek, Michal, linux-kernel, linux-phy, linux-arm-kernel, git (AMD-Xilinx), Paladugu, Siva Durga Prasad On 20-09-22, 10:38, Mehta, Piyush wrote: > Hello @Vinod Koul, > > Thanks for review comments. > Please find my inline comments below with tag [Piyush]. Please _do_ _not_ _top_ _post_ And learn how to reply upstream. Am sure there at people at AMD/Xilinx who know this.. > > Regards, > Piyush Mehta > > > -----Original Message----- > > From: Vinod Koul <vkoul@kernel.org> > > Sent: Tuesday, September 20, 2022 12:11 PM > > To: Mehta, Piyush <piyush.mehta@amd.com> > > Cc: laurent.pinchart@ideasonboard.com; kishon@ti.com; > > michal.simek@xilinx.com; Simek, Michal <michal.simek@amd.com>; linux- > > kernel@vger.kernel.org; linux-phy@lists.infradead.org; linux-arm- > > kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; Paladugu, Siva > > Durga Prasad <siva.durga.prasad.paladugu@amd.com> > > Subject: Re: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for > > power-save > > > > On 09-09-22, 18:34, Piyush Mehta wrote: > > > 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. > > > > > > > > > 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. > > > > > > 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 <piyush.mehta@amd.com> > > > --- > > > 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(>r_dev->gtr_mutex); > > > > > > + /* Configure and enable the clock when peripheral phy_init call */ > > > > when peripheral invokes phy_init > > > > > + 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) > > > 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. Learn to use a proper Linux MUA while replying on mail lists on you comment: 1. leaving clock on all the time does not sound right to me 2. why not enable pm_runtime for this driver and do better with power 3. Changes need to be documented in changelog, this was not explained in log so unexpected > > > > > > > > > icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0); > > > icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1); @@ -869,12 +866,6 > > @@ > > > 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) > > > 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", > > > - 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) > > > 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); > > @@ > > > -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) > > > provider = devm_of_phy_provider_register(&pdev->dev, > > xpsgtr_xlate); > > > 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 -- ~Vinod _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save 2022-09-20 11:49 ` Vinod Koul @ 2022-12-07 9:21 ` Mehta, Piyush 0 siblings, 0 replies; 6+ messages in thread From: Mehta, Piyush @ 2022-12-07 9:21 UTC (permalink / raw) To: Vinod Koul Cc: laurent.pinchart, kishon, michal.simek, Simek, Michal, linux-kernel, linux-phy, linux-arm-kernel, git (AMD-Xilinx), Paladugu, Siva Durga Prasad > -----Original Message----- > From: Vinod Koul <vkoul@kernel.org> > Sent: Tuesday, September 20, 2022 5:20 PM > To: Mehta, Piyush <piyush.mehta@amd.com> > Cc: laurent.pinchart@ideasonboard.com; kishon@ti.com; > michal.simek@xilinx.com; Simek, Michal <michal.simek@amd.com>; linux- > kernel@vger.kernel.org; linux-phy@lists.infradead.org; linux-arm- > kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; Paladugu, Siva > Durga Prasad <siva.durga.prasad.paladugu@amd.com> > Subject: Re: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for > power-save > > On 20-09-22, 10:38, Mehta, Piyush wrote: > > Hello @Vinod Koul, > > > > Thanks for review comments. > > Please find my inline comments below with tag [Piyush]. > > Please _do_ _not_ _top_ _post_ > > And learn how to reply upstream. Am sure there at people at AMD/Xilinx > who know this.. > > > > > > Regards, > > Piyush Mehta > > > > > -----Original Message----- > > > From: Vinod Koul <vkoul@kernel.org> > > > Sent: Tuesday, September 20, 2022 12:11 PM > > > To: Mehta, Piyush <piyush.mehta@amd.com> > > > Cc: laurent.pinchart@ideasonboard.com; kishon@ti.com; > > > michal.simek@xilinx.com; Simek, Michal <michal.simek@amd.com>; > > > linux- kernel@vger.kernel.org; linux-phy@lists.infradead.org; > > > linux-arm- kernel@lists.infradead.org; git (AMD-Xilinx) > > > <git@amd.com>; Paladugu, Siva Durga Prasad > > > <siva.durga.prasad.paladugu@amd.com> > > > Subject: Re: [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support > > > for power-save > > > > > > On 09-09-22, 18:34, Piyush Mehta wrote: > > > > 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. > > > > > > > > > > > > > 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. > > > > > > > > > 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 <piyush.mehta@amd.com> > > > > --- > > > > 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(>r_dev->gtr_mutex); > > > > > > > > + /* Configure and enable the clock when peripheral phy_init call > > > > +*/ > > > > > > when peripheral invokes phy_init > > > > > > > + 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) > > > > 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. > > Learn to use a proper Linux MUA while replying on mail lists > > on you comment: > 1. leaving clock on all the time does not sound right to me 2. why not enable > pm_runtime for this driver and do better with power Enable pm_runtime support: We have a fixed clock for the GT lanes, as earlier explained GT specific clock is enabled on phy initialization of the active state of peripheral (node status = "Okay"), and disabled on exit. For pm_runtime design support: static int __maybe_unused xpsgtr_runtime_suspend(struct device *dev) { return 0; } static int __maybe_unused xpsgtr_runtime_resume(struct device *dev) { return 0; } static int __maybe_unused xpsgtr_runtime_idle(struct device *dev) { pm_runtime_mark_last_busy(dev); pm_runtime_autosuspend(dev); return 0; } static const struct dev_pm_ops xpsgtr_pm_ops = { SET_RUNTIME_PM_OPS(xpsgtr_runtime_suspend, xpsgtr_runtime_resume, xpsgtr_runtime_idle) }; We don't enable/disable the clock on suspend/resume calls, so is it acceptable to have empty PM ops (xpsgtr_runtime_suspend / xpsgtr_runtime_resume)? 3. Changes need to be > documented in changelog, this was not explained in log so unexpected I will rephrase the commit description to have a detailed description. > > > > > > > > > > > > > > icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0); > > > > icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1); @@ -869,12 +866,6 > > > @@ > > > > 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) > > > > 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", > > > > - 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) > > > > 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); > > > @@ > > > > -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) > > > > provider = devm_of_phy_provider_register(&pdev->dev, > > > xpsgtr_xlate); > > > > 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 > > -- > ~Vinod Regards, Piyush Mehta _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-07 9:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-09 13:04 [PATCH] phy: xilinx: phy-zynqmp: dynamic clock support for power-save Piyush Mehta 2022-09-12 5:59 ` Michal Simek 2022-09-20 6:41 ` Vinod Koul 2022-09-20 10:38 ` Mehta, Piyush 2022-09-20 11:49 ` Vinod Koul 2022-12-07 9:21 ` Mehta, Piyush
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).