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 <[email protected]>
---
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;
/* 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]);
+
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;
+ }
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)
if (!clk)
continue;
+ gtr_dev->clk[refclk] = clk;
+
+ ret = clk_prepare_enable(gtr_dev->clk[refclk]);
+ if (ret)
+ return ret;
+
/*
* 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;
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[] = {
--
2.1.1
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 <[email protected]>
> ---
> 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.
>
> /* 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().
> +
> 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.
> + }
>
> 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.
> +
> /*
> * 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
Hi Laurent,
Thank you for the review.
> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Tuesday, March 9, 2021 1:37 AM
> To: Manish Narani <[email protected]>
> Cc: Anurag Kumar Vulisha <[email protected]>; [email protected];
> [email protected]; Michal Simek <[email protected]>; linux-
> [email protected]; [email protected]
> 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 <[email protected]>
> > ---
> > 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