[PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver

[PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver

[Adrien Thierry]

This series contains a few clock fixes for the qcom-snps-femto-v2 driver:
- make sure ref_clk is properly enabled
- add system sleep PM ops to disable the clocks on system suspend

v1 -> v2
- keep cfg_ahb clock and use clk_bulk API to handle both cfg_ahb and ref
  clocks (Bjorn Andersson)
- add system sleep PM callbacks (Bjorn Andersson)
- add Link: and Fixes: tag (Andrew Halaney)

Adrien Thierry (2):
  phy: qcom-snps-femto-v2: properly enable ref clock
  phy: qcom-snps-femto-v2: add system sleep PM ops

 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 85 +++++++++++++------
 1 file changed, 59 insertions(+), 26 deletions(-)

-- 
2.40.1

[PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock

[Adrien Thierry]

The driver is not enabling the ref clock, which thus gets disabled by
the clk_disable_unused initcall. This leads to the dwc3 controller
failing to initialize if probed after clk_disable_unused is called, for
instance when the driver is built as a module.

To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref
clocks at the proper places.

Note that the cfg_ahb clock is currently not used by any device tree
instantiation of the PHY. Work needs to be done separately to fix this.

Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/
Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs")
Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++-----
 1 file changed, 49 insertions(+), 18 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
index 6c237f3cc66d..ce1d2f8b568a 100644
--- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
+++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
@@ -110,11 +110,13 @@ struct phy_override_seq {
 /**
  * struct qcom_snps_hsphy - snps hs phy attributes
  *
+ * @dev: device structure
+ *
  * @phy: generic phy
  * @base: iomapped memory space for snps hs phy
  *
- * @cfg_ahb_clk: AHB2PHY interface clock
- * @ref_clk: phy reference clock
+ * @num_clks: number of clocks
+ * @clks: array of clocks
  * @phy_reset: phy reset control
  * @vregs: regulator supplies bulk data
  * @phy_initialized: if PHY has been initialized correctly
@@ -122,11 +124,13 @@ struct phy_override_seq {
  * @update_seq_cfg: tuning parameters for phy init
  */
 struct qcom_snps_hsphy {
+	struct device *dev;
+
 	struct phy *phy;
 	void __iomem *base;
 
-	struct clk *cfg_ahb_clk;
-	struct clk *ref_clk;
+	int num_clks;
+	struct clk_bulk_data *clks;
 	struct reset_control *phy_reset;
 	struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS];
 
@@ -135,6 +139,32 @@ struct qcom_snps_hsphy {
 	struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
 };
 
+static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
+{
+	struct device *dev = hsphy->dev;
+
+	hsphy->num_clks = 2;
+	hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL);
+	if (!hsphy->clks)
+		return -ENOMEM;
+
+	/*
+	 * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device
+	 * tree instantiation of the PHY is using the clock. This needs to be fixed in order
+	 * for this code to be able to use devm_clk_bulk_get().
+	 */
+	hsphy->clks[0].id = "cfg_ahb";
+	hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb");
+
+	hsphy->clks[1].id = "ref";
+	hsphy->clks[1].clk = devm_clk_get(dev, "ref");
+	if (IS_ERR(hsphy->clks[1].clk))
+		return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk),
+				     "failed to get ref clk\n");
+
+	return 0;
+}
+
 static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
 						u32 mask, u32 val)
 {
@@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
 					   0, USB2_AUTO_RESUME);
 	}
 
-	clk_disable_unprepare(hsphy->cfg_ahb_clk);
+	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
 	return 0;
 }
 
@@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
 
 	dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n");
 
-	ret = clk_prepare_enable(hsphy->cfg_ahb_clk);
+	ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
 	if (ret) {
-		dev_err(&hsphy->phy->dev, "failed to enable cfg ahb clock\n");
+		dev_err(&hsphy->phy->dev, "failed to enable clocks\n");
 		return ret;
 	}
 
@@ -374,16 +404,16 @@ static int qcom_snps_hsphy_init(struct phy *phy)
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(hsphy->cfg_ahb_clk);
+	ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
 	if (ret) {
-		dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret);
+		dev_err(&phy->dev, "failed to enable clocks, %d\n", ret);
 		goto poweroff_phy;
 	}
 
 	ret = reset_control_assert(hsphy->phy_reset);
 	if (ret) {
 		dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret);
-		goto disable_ahb_clk;
+		goto disable_clks;
 	}
 
 	usleep_range(100, 150);
@@ -391,7 +421,7 @@ static int qcom_snps_hsphy_init(struct phy *phy)
 	ret = reset_control_deassert(hsphy->phy_reset);
 	if (ret) {
 		dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret);
-		goto disable_ahb_clk;
+		goto disable_clks;
 	}
 
 	qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_CFG0,
@@ -448,8 +478,8 @@ static int qcom_snps_hsphy_init(struct phy *phy)
 
 	return 0;
 
-disable_ahb_clk:
-	clk_disable_unprepare(hsphy->cfg_ahb_clk);
+disable_clks:
+	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
 poweroff_phy:
 	regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
 
@@ -461,7 +491,7 @@ static int qcom_snps_hsphy_exit(struct phy *phy)
 	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
 
 	reset_control_assert(hsphy->phy_reset);
-	clk_disable_unprepare(hsphy->cfg_ahb_clk);
+	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
 	regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
 	hsphy->phy_initialized = false;
 
@@ -554,14 +584,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
 	if (!hsphy)
 		return -ENOMEM;
 
+	hsphy->dev = dev;
+
 	hsphy->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(hsphy->base))
 		return PTR_ERR(hsphy->base);
 
-	hsphy->ref_clk = devm_clk_get(dev, "ref");
-	if (IS_ERR(hsphy->ref_clk))
-		return dev_err_probe(dev, PTR_ERR(hsphy->ref_clk),
-				     "failed to get ref clk\n");
+	ret = qcom_snps_hsphy_clk_init(hsphy);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize clocks\n");
 
 	hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 	if (IS_ERR(hsphy->phy_reset)) {
-- 
2.40.1

Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock

[Konrad Dybcio]

On 5.06.2023 20:44, Adrien Thierry wrote:
> The driver is not enabling the ref clock, which thus gets disabled by
> the clk_disable_unused initcall. This leads to the dwc3 controller
> failing to initialize if probed after clk_disable_unused is called, for
> instance when the driver is built as a module.
> 
> To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref
> clocks at the proper places.
> 
> Note that the cfg_ahb clock is currently not used by any device tree
> instantiation of the PHY. Work needs to be done separately to fix this.
> 
> Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/
> Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs")
> Signed-off-by: Adrien Thierry <athierry@redhat.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++-----
>  1 file changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> index 6c237f3cc66d..ce1d2f8b568a 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> @@ -110,11 +110,13 @@ struct phy_override_seq {
>  /**
>   * struct qcom_snps_hsphy - snps hs phy attributes
>   *
> + * @dev: device structure
> + *
>   * @phy: generic phy
>   * @base: iomapped memory space for snps hs phy
>   *
> - * @cfg_ahb_clk: AHB2PHY interface clock
> - * @ref_clk: phy reference clock
> + * @num_clks: number of clocks
> + * @clks: array of clocks
>   * @phy_reset: phy reset control
>   * @vregs: regulator supplies bulk data
>   * @phy_initialized: if PHY has been initialized correctly
> @@ -122,11 +124,13 @@ struct phy_override_seq {
>   * @update_seq_cfg: tuning parameters for phy init
>   */
>  struct qcom_snps_hsphy {
> +	struct device *dev;
> +
>  	struct phy *phy;
>  	void __iomem *base;
>  
> -	struct clk *cfg_ahb_clk;
> -	struct clk *ref_clk;
> +	int num_clks;
> +	struct clk_bulk_data *clks;
>  	struct reset_control *phy_reset;
>  	struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS];
>  
> @@ -135,6 +139,32 @@ struct qcom_snps_hsphy {
>  	struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
>  };
>  
> +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
> +{
> +	struct device *dev = hsphy->dev;
> +
> +	hsphy->num_clks = 2;
> +	hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL);
> +	if (!hsphy->clks)
> +		return -ENOMEM;
> +
> +	/*
> +	 * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device
> +	 * tree instantiation of the PHY is using the clock. This needs to be fixed in order
> +	 * for this code to be able to use devm_clk_bulk_get().
> +	 */
> +	hsphy->clks[0].id = "cfg_ahb";
> +	hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb");
Hm, maybe you could first check if we can get this clock
properly (!IS_ERR_OR_NULL) and then allocate the second
slot..

> +
> +	hsphy->clks[1].id = "ref";
> +	hsphy->clks[1].clk = devm_clk_get(dev, "ref");
> +	if (IS_ERR(hsphy->clks[1].clk))
> +		return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk),
> +				     "failed to get ref clk\n");
> +
> +	return 0;
> +}
> +
>  static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
>  						u32 mask, u32 val)
>  {
> @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
>  					   0, USB2_AUTO_RESUME);
>  	}
>  
> -	clk_disable_unprepare(hsphy->cfg_ahb_clk);
> +	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>  	return 0;
>  }
>  
> @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
>  
>  	dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n");
>  
> -	ret = clk_prepare_enable(hsphy->cfg_ahb_clk);
> +	ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
Aren't you dereferencing NULL if the optional clock is absent?

Konrad
>  	if (ret) {
> -		dev_err(&hsphy->phy->dev, "failed to enable cfg ahb clock\n");
> +		dev_err(&hsphy->phy->dev, "failed to enable clocks\n");
>  		return ret;
>  	}
>  
> @@ -374,16 +404,16 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_prepare_enable(hsphy->cfg_ahb_clk);
> +	ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
>  	if (ret) {
> -		dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret);
> +		dev_err(&phy->dev, "failed to enable clocks, %d\n", ret);
>  		goto poweroff_phy;
>  	}
>  
>  	ret = reset_control_assert(hsphy->phy_reset);
>  	if (ret) {
>  		dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret);
> -		goto disable_ahb_clk;
> +		goto disable_clks;
>  	}
>  
>  	usleep_range(100, 150);
> @@ -391,7 +421,7 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>  	ret = reset_control_deassert(hsphy->phy_reset);
>  	if (ret) {
>  		dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret);
> -		goto disable_ahb_clk;
> +		goto disable_clks;
>  	}
>  
>  	qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_CFG0,
> @@ -448,8 +478,8 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>  
>  	return 0;
>  
> -disable_ahb_clk:
> -	clk_disable_unprepare(hsphy->cfg_ahb_clk);
> +disable_clks:
> +	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>  poweroff_phy:
>  	regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>  
> @@ -461,7 +491,7 @@ static int qcom_snps_hsphy_exit(struct phy *phy)
>  	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
>  
>  	reset_control_assert(hsphy->phy_reset);
> -	clk_disable_unprepare(hsphy->cfg_ahb_clk);
> +	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>  	regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>  	hsphy->phy_initialized = false;
>  
> @@ -554,14 +584,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>  	if (!hsphy)
>  		return -ENOMEM;
>  
> +	hsphy->dev = dev;
> +
>  	hsphy->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(hsphy->base))
>  		return PTR_ERR(hsphy->base);
>  
> -	hsphy->ref_clk = devm_clk_get(dev, "ref");
> -	if (IS_ERR(hsphy->ref_clk))
> -		return dev_err_probe(dev, PTR_ERR(hsphy->ref_clk),
> -				     "failed to get ref clk\n");
> +	ret = qcom_snps_hsphy_clk_init(hsphy);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize clocks\n");
>  
>  	hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>  	if (IS_ERR(hsphy->phy_reset)) {

Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock

[Andrew Halaney]

On Tue, Jun 06, 2023 at 01:14:00AM +0200, Konrad Dybcio wrote:
> 
> 
> On 5.06.2023 20:44, Adrien Thierry wrote:
> > The driver is not enabling the ref clock, which thus gets disabled by
> > the clk_disable_unused initcall. This leads to the dwc3 controller
> > failing to initialize if probed after clk_disable_unused is called, for
> > instance when the driver is built as a module.
> > 
> > To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref
> > clocks at the proper places.
> > 
> > Note that the cfg_ahb clock is currently not used by any device tree
> > instantiation of the PHY. Work needs to be done separately to fix this.
> > 
> > Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/
> > Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs")
> > Signed-off-by: Adrien Thierry <athierry@redhat.com>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++-----
> >  1 file changed, 49 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > index 6c237f3cc66d..ce1d2f8b568a 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > @@ -110,11 +110,13 @@ struct phy_override_seq {
> >  /**
> >   * struct qcom_snps_hsphy - snps hs phy attributes
> >   *
> > + * @dev: device structure
> > + *
> >   * @phy: generic phy
> >   * @base: iomapped memory space for snps hs phy
> >   *
> > - * @cfg_ahb_clk: AHB2PHY interface clock
> > - * @ref_clk: phy reference clock
> > + * @num_clks: number of clocks
> > + * @clks: array of clocks
> >   * @phy_reset: phy reset control
> >   * @vregs: regulator supplies bulk data
> >   * @phy_initialized: if PHY has been initialized correctly
> > @@ -122,11 +124,13 @@ struct phy_override_seq {
> >   * @update_seq_cfg: tuning parameters for phy init
> >   */
> >  struct qcom_snps_hsphy {
> > +	struct device *dev;
> > +
> >  	struct phy *phy;
> >  	void __iomem *base;
> >  
> > -	struct clk *cfg_ahb_clk;
> > -	struct clk *ref_clk;
> > +	int num_clks;
> > +	struct clk_bulk_data *clks;
> >  	struct reset_control *phy_reset;
> >  	struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS];
> >  
> > @@ -135,6 +139,32 @@ struct qcom_snps_hsphy {
> >  	struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
> >  };
> >  
> > +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
> > +{
> > +	struct device *dev = hsphy->dev;
> > +
> > +	hsphy->num_clks = 2;
> > +	hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL);
> > +	if (!hsphy->clks)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device
> > +	 * tree instantiation of the PHY is using the clock. This needs to be fixed in order
> > +	 * for this code to be able to use devm_clk_bulk_get().
> > +	 */
> > +	hsphy->clks[0].id = "cfg_ahb";
> > +	hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb");
> Hm, maybe you could first check if we can get this clock
> properly (!IS_ERR_OR_NULL) and then allocate the second
> slot..
> 

The bulk clk api handles NULL clks without issue if I am reading right,
so personally if we're going to use the bulk api I say we carry the extra
slot unconditionally. No expert on this stuff but that seems more
straightforward. Honestly I wouldn't mind using the bulk optional API,
then checking the "non optional ref clock" manually. That's closer to
the ideal flow to me. Super opinionated though, don't take my word as
right.

> > +
> > +	hsphy->clks[1].id = "ref";
> > +	hsphy->clks[1].clk = devm_clk_get(dev, "ref");
> > +	if (IS_ERR(hsphy->clks[1].clk))
> > +		return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk),
> > +				     "failed to get ref clk\n");
> > +
> > +	return 0;
> > +}
> > +
> >  static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
> >  						u32 mask, u32 val)
> >  {
> > @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
> >  					   0, USB2_AUTO_RESUME);
> >  	}
> >  
> > -	clk_disable_unprepare(hsphy->cfg_ahb_clk);
> > +	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
> >  	return 0;
> >  }
> >  
> > @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
> >  
> >  	dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n");
> >  
> > -	ret = clk_prepare_enable(hsphy->cfg_ahb_clk);
> > +	ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
> Aren't you dereferencing NULL if the optional clock is absent?
> 

Similar to above, the bulk api seems to handle NULL clks gracefully.

Thanks,
Andrew

Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock

[Adrien Thierry]

Thanks for your feedback Konrad and Andrew!

On Tue, Jun 06, 2023 at 08:55:16AM -0500, Andrew Halaney wrote:
> On Tue, Jun 06, 2023 at 01:14:00AM +0200, Konrad Dybcio wrote:
> > 
> > 
> > On 5.06.2023 20:44, Adrien Thierry wrote:
> > > The driver is not enabling the ref clock, which thus gets disabled by
> > > the clk_disable_unused initcall. This leads to the dwc3 controller
> > > failing to initialize if probed after clk_disable_unused is called, for
> > > instance when the driver is built as a module.
> > > 
> > > To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref
> > > clocks at the proper places.
> > > 
> > > Note that the cfg_ahb clock is currently not used by any device tree
> > > instantiation of the PHY. Work needs to be done separately to fix this.
> > > 
> > > Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/
> > > Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs")
> > > Signed-off-by: Adrien Thierry <athierry@redhat.com>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++-----
> > >  1 file changed, 49 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > > index 6c237f3cc66d..ce1d2f8b568a 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> > > @@ -110,11 +110,13 @@ struct phy_override_seq {
> > >  /**
> > >   * struct qcom_snps_hsphy - snps hs phy attributes
> > >   *
> > > + * @dev: device structure
> > > + *
> > >   * @phy: generic phy
> > >   * @base: iomapped memory space for snps hs phy
> > >   *
> > > - * @cfg_ahb_clk: AHB2PHY interface clock
> > > - * @ref_clk: phy reference clock
> > > + * @num_clks: number of clocks
> > > + * @clks: array of clocks
> > >   * @phy_reset: phy reset control
> > >   * @vregs: regulator supplies bulk data
> > >   * @phy_initialized: if PHY has been initialized correctly
> > > @@ -122,11 +124,13 @@ struct phy_override_seq {
> > >   * @update_seq_cfg: tuning parameters for phy init
> > >   */
> > >  struct qcom_snps_hsphy {
> > > +	struct device *dev;
> > > +
> > >  	struct phy *phy;
> > >  	void __iomem *base;
> > >  
> > > -	struct clk *cfg_ahb_clk;
> > > -	struct clk *ref_clk;
> > > +	int num_clks;
> > > +	struct clk_bulk_data *clks;
> > >  	struct reset_control *phy_reset;
> > >  	struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS];
> > >  
> > > @@ -135,6 +139,32 @@ struct qcom_snps_hsphy {
> > >  	struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
> > >  };
> > >  
> > > +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
> > > +{
> > > +	struct device *dev = hsphy->dev;
> > > +
> > > +	hsphy->num_clks = 2;
> > > +	hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL);
> > > +	if (!hsphy->clks)
> > > +		return -ENOMEM;
> > > +
> > > +	/*
> > > +	 * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device
> > > +	 * tree instantiation of the PHY is using the clock. This needs to be fixed in order
> > > +	 * for this code to be able to use devm_clk_bulk_get().
> > > +	 */
> > > +	hsphy->clks[0].id = "cfg_ahb";
> > > +	hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb");
> > Hm, maybe you could first check if we can get this clock
> > properly (!IS_ERR_OR_NULL) and then allocate the second
> > slot..
> > 
> 
> The bulk clk api handles NULL clks without issue if I am reading right,
> so personally if we're going to use the bulk api I say we carry the extra
> slot unconditionally. No expert on this stuff but that seems more
> straightforward. Honestly I wouldn't mind using the bulk optional API,
> then checking the "non optional ref clock" manually. That's closer to
> the ideal flow to me. Super opinionated though, don't take my word as
> right.
>

Agree with Andrew. Since cfg_ahb is always NULL, I'm certainly "wasting"
an array cell here but I think it also better highlights the fact that
it's a hack and that cfg_ahb needs to be properly wired in the DTs. As for
using the bulk optional API, I'm fine with both!

> > > +
> > > +	hsphy->clks[1].id = "ref";
> > > +	hsphy->clks[1].clk = devm_clk_get(dev, "ref");
> > > +	if (IS_ERR(hsphy->clks[1].clk))
> > > +		return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk),
> > > +				     "failed to get ref clk\n");
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
> > >  						u32 mask, u32 val)
> > >  {
> > > @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
> > >  					   0, USB2_AUTO_RESUME);
> > >  	}
> > >  
> > > -	clk_disable_unprepare(hsphy->cfg_ahb_clk);
> > > +	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
> > >  	return 0;
> > >  }
> > >  
> > > @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
> > >  
> > >  	dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n");
> > >  
> > > -	ret = clk_prepare_enable(hsphy->cfg_ahb_clk);
> > > +	ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
> > Aren't you dereferencing NULL if the optional clock is absent?
> > 
> 
> Similar to above, the bulk api seems to handle NULL clks gracefully.
>

devm_clk_get_optional() will return NULL for cfg_ahb, but AFAIU NULL
serves as a dummy clock [1] with which the clock API deals gracefully. The
various functions like clk_prepare(), clk_enable() check if the clock is
NULL and return 0 immediately if that's the case (see for instance [2]).

[1] https://elixir.bootlin.com/linux/v6.4-rc5/source/include/linux/clk.h#L514
[2] https://elixir.bootlin.com/linux/v6.4-rc5/source/drivers/clk/clk.c#L1045

Best,
Adrien

> Thanks,
> Andrew
> 

Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock

[Konrad Dybcio]

On 6.06.2023 16:35, Adrien Thierry wrote:
> Thanks for your feedback Konrad and Andrew!
> 
> On Tue, Jun 06, 2023 at 08:55:16AM -0500, Andrew Halaney wrote:
>> On Tue, Jun 06, 2023 at 01:14:00AM +0200, Konrad Dybcio wrote:
>>>
>>>
>>> On 5.06.2023 20:44, Adrien Thierry wrote:
>>>> The driver is not enabling the ref clock, which thus gets disabled by
>>>> the clk_disable_unused initcall. This leads to the dwc3 controller
>>>> failing to initialize if probed after clk_disable_unused is called, for
>>>> instance when the driver is built as a module.
>>>>
>>>> To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref
>>>> clocks at the proper places.
>>>>
>>>> Note that the cfg_ahb clock is currently not used by any device tree
>>>> instantiation of the PHY. Work needs to be done separately to fix this.
>>>>
>>>> Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/
>>>> Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs")
>>>> Signed-off-by: Adrien Thierry <athierry@redhat.com>
>>>> ---
>>>>  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++-----
>>>>  1 file changed, 49 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>>>> index 6c237f3cc66d..ce1d2f8b568a 100644
>>>> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>>>> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>>>> @@ -110,11 +110,13 @@ struct phy_override_seq {
>>>>  /**
>>>>   * struct qcom_snps_hsphy - snps hs phy attributes
>>>>   *
>>>> + * @dev: device structure
>>>> + *
>>>>   * @phy: generic phy
>>>>   * @base: iomapped memory space for snps hs phy
>>>>   *
>>>> - * @cfg_ahb_clk: AHB2PHY interface clock
>>>> - * @ref_clk: phy reference clock
>>>> + * @num_clks: number of clocks
>>>> + * @clks: array of clocks
>>>>   * @phy_reset: phy reset control
>>>>   * @vregs: regulator supplies bulk data
>>>>   * @phy_initialized: if PHY has been initialized correctly
>>>> @@ -122,11 +124,13 @@ struct phy_override_seq {
>>>>   * @update_seq_cfg: tuning parameters for phy init
>>>>   */
>>>>  struct qcom_snps_hsphy {
>>>> +	struct device *dev;
>>>> +
>>>>  	struct phy *phy;
>>>>  	void __iomem *base;
>>>>  
>>>> -	struct clk *cfg_ahb_clk;
>>>> -	struct clk *ref_clk;
>>>> +	int num_clks;
>>>> +	struct clk_bulk_data *clks;
>>>>  	struct reset_control *phy_reset;
>>>>  	struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS];
>>>>  
>>>> @@ -135,6 +139,32 @@ struct qcom_snps_hsphy {
>>>>  	struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
>>>>  };
>>>>  
>>>> +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
>>>> +{
>>>> +	struct device *dev = hsphy->dev;
>>>> +
>>>> +	hsphy->num_clks = 2;
>>>> +	hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL);
>>>> +	if (!hsphy->clks)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	/*
>>>> +	 * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device
>>>> +	 * tree instantiation of the PHY is using the clock. This needs to be fixed in order
>>>> +	 * for this code to be able to use devm_clk_bulk_get().
>>>> +	 */
>>>> +	hsphy->clks[0].id = "cfg_ahb";
>>>> +	hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb");
>>> Hm, maybe you could first check if we can get this clock
>>> properly (!IS_ERR_OR_NULL) and then allocate the second
>>> slot..
>>>
>>
>> The bulk clk api handles NULL clks without issue if I am reading right,
>> so personally if we're going to use the bulk api I say we carry the extra
>> slot unconditionally. No expert on this stuff but that seems more
>> straightforward. Honestly I wouldn't mind using the bulk optional API,
>> then checking the "non optional ref clock" manually. That's closer to
>> the ideal flow to me. Super opinionated though, don't take my word as
>> right.
>>
> 
> Agree with Andrew. Since cfg_ahb is always NULL, I'm certainly "wasting"
> an array cell here but I think it also better highlights the fact that
> it's a hack and that cfg_ahb needs to be properly wired in the DTs. As for
> using the bulk optional API, I'm fine with both!
Hm right, let's keep it as-is so that the other clock which is
actually necessary never gets skipped accidentally..

Konrad
> 
>>>> +
>>>> +	hsphy->clks[1].id = "ref";
>>>> +	hsphy->clks[1].clk = devm_clk_get(dev, "ref");
>>>> +	if (IS_ERR(hsphy->clks[1].clk))
>>>> +		return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk),
>>>> +				     "failed to get ref clk\n");
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
>>>>  						u32 mask, u32 val)
>>>>  {
>>>> @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
>>>>  					   0, USB2_AUTO_RESUME);
>>>>  	}
>>>>  
>>>> -	clk_disable_unprepare(hsphy->cfg_ahb_clk);
>>>> +	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
>>>>  
>>>>  	dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n");
>>>>  
>>>> -	ret = clk_prepare_enable(hsphy->cfg_ahb_clk);
>>>> +	ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
>>> Aren't you dereferencing NULL if the optional clock is absent?
>>>
>>
>> Similar to above, the bulk api seems to handle NULL clks gracefully.
>>
> 
> devm_clk_get_optional() will return NULL for cfg_ahb, but AFAIU NULL
> serves as a dummy clock [1] with which the clock API deals gracefully. The
> various functions like clk_prepare(), clk_enable() check if the clock is
> NULL and return 0 immediately if that's the case (see for instance [2]).
> 
> [1] https://elixir.bootlin.com/linux/v6.4-rc5/source/include/linux/clk.h#L514
> [2] https://elixir.bootlin.com/linux/v6.4-rc5/source/drivers/clk/clk.c#L1045
> 
> Best,
> Adrien
> 
>> Thanks,
>> Andrew
>>
> 

Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock

[Andrew Halaney]

On Mon, Jun 05, 2023 at 02:44:53PM -0400, Adrien Thierry wrote:
> The driver is not enabling the ref clock, which thus gets disabled by
> the clk_disable_unused initcall. This leads to the dwc3 controller
> failing to initialize if probed after clk_disable_unused is called, for
> instance when the driver is built as a module.
> 
> To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref
> clocks at the proper places.
> 
> Note that the cfg_ahb clock is currently not used by any device tree
> instantiation of the PHY. Work needs to be done separately to fix this.
> 
> Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/
> Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs")
> Signed-off-by: Adrien Thierry <athierry@redhat.com>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

[PATCH v2 2/2] phy: qcom-snps-femto-v2: add system sleep PM ops

[Adrien Thierry]

Add the system sleep suspend and resume callbacks, reusing the same code
as the runtime suspend and resume callbacks.

Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
I'm still a bit confused as to what the difference is between
suspend/resume PM ops and the struct usb_phy set_suspend() callback.
Please tell me if I should be populating the latter instead.

 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
index ce1d2f8b568a..378a5029f61e 100644
--- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
+++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
@@ -179,7 +179,7 @@ static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
 	readl_relaxed(base + offset);
 }
 
-static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
+static int qcom_snps_hsphy_do_suspend(struct qcom_snps_hsphy *hsphy)
 {
 	dev_dbg(&hsphy->phy->dev, "Suspend QCOM SNPS PHY\n");
 
@@ -199,7 +199,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
 	return 0;
 }
 
-static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
+static int qcom_snps_hsphy_do_resume(struct qcom_snps_hsphy *hsphy)
 {
 	int ret;
 
@@ -214,25 +214,25 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
 	return 0;
 }
 
-static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device *dev)
+static int __maybe_unused qcom_snps_hsphy_suspend(struct device *dev)
 {
 	struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev);
 
 	if (!hsphy->phy_initialized)
 		return 0;
 
-	qcom_snps_hsphy_suspend(hsphy);
+	qcom_snps_hsphy_do_suspend(hsphy);
 	return 0;
 }
 
-static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev)
+static int __maybe_unused qcom_snps_hsphy_resume(struct device *dev)
 {
 	struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev);
 
 	if (!hsphy->phy_initialized)
 		return 0;
 
-	qcom_snps_hsphy_resume(hsphy);
+	qcom_snps_hsphy_do_resume(hsphy);
 	return 0;
 }
 
@@ -518,8 +518,10 @@ static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
 MODULE_DEVICE_TABLE(of, qcom_snps_hsphy_of_match_table);
 
 static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
-	SET_RUNTIME_PM_OPS(qcom_snps_hsphy_runtime_suspend,
-			   qcom_snps_hsphy_runtime_resume, NULL)
+	SET_RUNTIME_PM_OPS(qcom_snps_hsphy_suspend,
+			   qcom_snps_hsphy_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(qcom_snps_hsphy_suspend,
+				qcom_snps_hsphy_resume)
 };
 
 static void qcom_snps_hsphy_override_param_update_val(
-- 
2.40.1

Re: [PATCH v2 2/2] phy: qcom-snps-femto-v2: add system sleep PM ops

[Andrew Halaney]

On Mon, Jun 05, 2023 at 02:44:54PM -0400, Adrien Thierry wrote:
> Add the system sleep suspend and resume callbacks, reusing the same code
> as the runtime suspend and resume callbacks.

It would be nice (imo) to reference the downstream driver you used as a
reference if you spin a v3.

> 
> Signed-off-by: Adrien Thierry <athierry@redhat.com>
> ---
> I'm still a bit confused as to what the difference is between
> suspend/resume PM ops and the struct usb_phy set_suspend() callback.
> Please tell me if I should be populating the latter instead.

My understanding is usb_phy is a legacy thing, and that the generic phys
are the way to go (which this is).

Looking at dwc3 for example, you can see it uses:

	phy_power_off(dwc->usb3_generic_phy);
	phy_power_off(dwc->usb2_generic_phy);

	usb_phy_set_suspend(dwc->usb3_phy, 1);
	usb_phy_set_suspend(dwc->usb2_phy, 1);

so it handles both the generic phy (like this) and the usb_phy.
This driver doesn't implement the power_off callback, but phy_power_off
is also pm_runtime aware, so calling phy_power_off in a controller still
achieves similar behavior (although I'm following right
phy_init/phy_exit also are pm_runtime aware, so I think it would be at
that point that the ops get called).

This all assumes a user enables pm_runtime in sysfs for this driver.

> 
>  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> index ce1d2f8b568a..378a5029f61e 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> @@ -179,7 +179,7 @@ static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
>  	readl_relaxed(base + offset);
>  }
>  
> -static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
> +static int qcom_snps_hsphy_do_suspend(struct qcom_snps_hsphy *hsphy)
>  {
>  	dev_dbg(&hsphy->phy->dev, "Suspend QCOM SNPS PHY\n");
>  
> @@ -199,7 +199,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
>  	return 0;
>  }
>  
> -static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
> +static int qcom_snps_hsphy_do_resume(struct qcom_snps_hsphy *hsphy)
>  {
>  	int ret;
>  
> @@ -214,25 +214,25 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
>  	return 0;
>  }
>  
> -static int __maybe_unused qcom_snps_hsphy_runtime_suspend(struct device *dev)
> +static int __maybe_unused qcom_snps_hsphy_suspend(struct device *dev)
>  {
>  	struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev);
>  
>  	if (!hsphy->phy_initialized)
>  		return 0;
>  
> -	qcom_snps_hsphy_suspend(hsphy);
> +	qcom_snps_hsphy_do_suspend(hsphy);

Deserves its own patch, but can you return qcom_snps_hsphy_do_suspend/resume
as part of your clean up in this series?

>  	return 0;
>  }
>  
> -static int __maybe_unused qcom_snps_hsphy_runtime_resume(struct device *dev)
> +static int __maybe_unused qcom_snps_hsphy_resume(struct device *dev)
>  {
>  	struct qcom_snps_hsphy *hsphy = dev_get_drvdata(dev);
>  
>  	if (!hsphy->phy_initialized)
>  		return 0;
>  
> -	qcom_snps_hsphy_resume(hsphy);
> +	qcom_snps_hsphy_do_resume(hsphy);
>  	return 0;
>  }
>  
> @@ -518,8 +518,10 @@ static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
>  MODULE_DEVICE_TABLE(of, qcom_snps_hsphy_of_match_table);
>  
>  static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
> -	SET_RUNTIME_PM_OPS(qcom_snps_hsphy_runtime_suspend,
> -			   qcom_snps_hsphy_runtime_resume, NULL)
> +	SET_RUNTIME_PM_OPS(qcom_snps_hsphy_suspend,
> +			   qcom_snps_hsphy_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(qcom_snps_hsphy_suspend,
> +				qcom_snps_hsphy_resume)
>  };
>  
>  static void qcom_snps_hsphy_override_param_update_val(
> -- 
> 2.40.1
> 

Re: [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver

[Adrien Thierry]

On Mon, Jun 05, 2023 at 02:44:52PM -0400, Adrien Thierry wrote:
> This series contains a few clock fixes for the qcom-snps-femto-v2 driver:
> - make sure ref_clk is properly enabled
> - add system sleep PM ops to disable the clocks on system suspend
> 
> v1 -> v2
> - keep cfg_ahb clock and use clk_bulk API to handle both cfg_ahb and ref
>   clocks (Bjorn Andersson)
> - add system sleep PM callbacks (Bjorn Andersson)
> - add Link: and Fixes: tag (Andrew Halaney)
> 
> Adrien Thierry (2):
>   phy: qcom-snps-femto-v2: properly enable ref clock
>   phy: qcom-snps-femto-v2: add system sleep PM ops
> 
>  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 85 +++++++++++++------
>  1 file changed, 59 insertions(+), 26 deletions(-)
> 
> -- 
> 2.40.1
>

Gentle ping Bjorn, is there anything you'd like me to change in a next
revision?

Best,
Adrien