lkml.org 
[lkml]   [2021]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] phy: zynqmp: Handle the clock enable/disable properly
Date
Hi Laurent,

Thank you for the review.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Tuesday, March 9, 2021 1:37 AM
> To: Manish Narani <MNARANI@xilinx.com>
> Cc: Anurag Kumar Vulisha <anuragku@xlnx.xilinx.com>; kishon@ti.com;
> vkoul@kernel.org; Michal Simek <michals@xilinx.com>; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] phy: zynqmp: Handle the clock enable/disable properly
>
> Hi Manish,
>
> Thank you for the patch.
>
> On Tue, Mar 09, 2021 at 12:19:16AM +0530, Manish Narani wrote:
> > The current driver is not handling the clock enable/disable operations
> > properly. The clocks need to be handled correctly by enabling or
> > disabling at appropriate places. This patch adds code to handle the
> > same.
> >
> > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > ---
> > drivers/phy/xilinx/phy-zynqmp.c | 40
> +++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 37 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
> zynqmp.c
> > index 2b65f84..0ec534e 100644
> > --- a/drivers/phy/xilinx/phy-zynqmp.c
> > +++ b/drivers/phy/xilinx/phy-zynqmp.c
> > @@ -219,6 +219,7 @@ struct xpsgtr_dev {
> > struct mutex gtr_mutex; /* mutex for locking */
> > struct xpsgtr_phy phys[NUM_LANES];
> > const struct xpsgtr_ssc *refclk_sscs[NUM_LANES];
> > + struct clk *clk[NUM_LANES];
> > bool tx_term_fix;
> > unsigned int saved_icm_cfg0;
> > unsigned int saved_icm_cfg1;
> > @@ -818,11 +819,15 @@ 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);
> > + int i;
>
> i is never negative, so you can make it an unsigned int.

OK. Will update in v2.

>
> >
> > /* 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(gtr_dev->clk[i]);
>
> Why is this only clk_disable(), and not clk_disable_unprepare() ? Same
> question for xpsgtr_resume().

It can be clk_disable_unprepare() and clk_prepare_enable in suspend() and resume() respectively, will update in v2.

>
> > +
> > return 0;
> > }
> >
> > @@ -832,6 +837,13 @@ 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_enable(gtr_dev->clk[i]);
> > + if (err)
> > + return err;
>
> In case of error we need to disable the clocks that have been
> successfully enabled already.

Thanks for bringing this. Will be updated in v2.

>
> > + }
> >
> > icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0);
> > icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1);
> > @@ -865,6 +877,7 @@ 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;
> > @@ -882,6 +895,12 @@ static int xpsgtr_get_ref_clocks(struct xpsgtr_dev
> *gtr_dev)
>
> There's an error check above that needs to jump to err_clk_put too.
>
> > if (!clk)
> > continue;
> >
> > + gtr_dev->clk[refclk] = clk;
> > +
> > + ret = clk_prepare_enable(gtr_dev->clk[refclk]);
> > + if (ret)
> > + return ret;
>
> It would be nice to move the driver to runtime PM to keep the clocks
> disabled when the PHY isn't in use. It can be done in a separate patch.

Sure thanks. I will plan for that as well.

Thanks,
Manish

>
> > +
> > /*
> > * Get the spread spectrum (SSC) settings for the reference
> > * clock rate.
> > @@ -899,11 +918,17 @@ 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);
> > - return -EINVAL;
> > + goto err_clk_put;
> > }
> > }
> >
> > return 0;
> > +
> > +err_clk_put:
> > + for (refclk = 0; refclk < ARRAY_SIZE(gtr_dev->clk); refclk++)
> > + clk_disable_unprepare(gtr_dev->clk[refclk]);
> > +
> > + return -EINVAL;
> > }
> >
> > static int xpsgtr_probe(struct platform_device *pdev)
> > @@ -913,6 +938,7 @@ static int xpsgtr_probe(struct platform_device
> *pdev)
> > struct phy_provider *provider;
> > unsigned int port;
> > int ret;
> > + int i;
>
> unsigned int here too.
>
> >
> > gtr_dev = devm_kzalloc(&pdev->dev, sizeof(*gtr_dev), GFP_KERNEL);
> > if (!gtr_dev)
> > @@ -951,7 +977,8 @@ 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");
> > - return PTR_ERR(phy);
> > + ret = PTR_ERR(phy);
> > + goto err_clk_put;
> > }
> >
> > gtr_phy->phy = phy;
> > @@ -962,9 +989,16 @@ 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");
> > - return PTR_ERR(provider);
> > + ret = PTR_ERR(provider);
> > + goto err_clk_put;
> > }
> > 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[] = {
>
> --
> Regards,
>
> Laurent Pinchart
\
 
 \ /
  Last update: 2021-03-09 11:13    [W:0.698 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site