RE: [PATCH] phy: zynqmp: Handle the clock enable/disable properly

From: Manish Narani <MNARANI@xilinx.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Anurag Kumar Vulisha <anuragku@xlnx.xilinx.com>,
	"kishon@ti.com" <kishon@ti.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	Michal Simek <michals@xilinx.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH] phy: zynqmp: Handle the clock enable/disable properly
Date: Tue, 9 Mar 2021 10:12:07 +0000	[thread overview]
Message-ID: <BYAPR02MB5896AE05855769536045162FC1929@BYAPR02MB5896.namprd02.prod.outlook.com> (raw)
In-Reply-To: <YEaDzWeakpJv4DaT@pendragon.ideasonboard.com>

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