[PATCH v3 0/3] Fixes for qcom-snps-femto-v2 PHY driver

[PATCH v3 0/3] Fixes for qcom-snps-femto-v2 PHY driver

[Adrien Thierry]

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

v2 -> v3
- "phy: qcom-snps-femto-v2: add system sleep PM ops" - add link to
  downstream driver used as reference (Andrew Halaney)
- add commit "phy: qcom-snps-femto-v2: use
  qcom_snps_hsphy_do_suspend/resume error code" to make sure PM ops don't
  always return 0 (Andrew Halaney)

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 (3):
  phy: qcom-snps-femto-v2: properly enable ref clock
  phy: qcom-snps-femto-v2: add system sleep PM ops
  phy: qcom-snps-femto-v2: use qcom_snps_hsphy_do_suspend/resume error
    code

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

-- 
2.40.1

[PATCH v3 1/3] 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")
Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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

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

[Adrien Thierry]

The downstream driver [1] implements set_suspend(), which deals with
both runtime and system sleep/resume. The upstream driver already has
runtime PM ops, so add the system sleep PM ops as well, reusing the same
code as the runtime PM ops.

[1] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c

Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
 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

[PATCH v3 3/3] phy: qcom-snps-femto-v2: use qcom_snps_hsphy_do_suspend/resume error code

[Adrien Thierry]

The return value from qcom_snps_hsphy_do_suspend/resume is not used.
Make sure qcom_snps_hsphy_suspend/resume return this value as well.

Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
index 378a5029f61e..6be8b5218aaa 100644
--- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
+++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
@@ -221,8 +221,7 @@ static int __maybe_unused qcom_snps_hsphy_suspend(struct device *dev)
 	if (!hsphy->phy_initialized)
 		return 0;
 
-	qcom_snps_hsphy_do_suspend(hsphy);
-	return 0;
+	return qcom_snps_hsphy_do_suspend(hsphy);
 }
 
 static int __maybe_unused qcom_snps_hsphy_resume(struct device *dev)
@@ -232,8 +231,7 @@ static int __maybe_unused qcom_snps_hsphy_resume(struct device *dev)
 	if (!hsphy->phy_initialized)
 		return 0;
 
-	qcom_snps_hsphy_do_resume(hsphy);
-	return 0;
+	return qcom_snps_hsphy_do_resume(hsphy);
 }
 
 static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode,
-- 
2.40.1

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

[Bjorn Andersson]

On Thu, Jun 22, 2023 at 03:40:18PM -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")
> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> 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);

This is ok, but it it wouldn't be wrong to just make clks[2] in the
context struct...

> +	if (!hsphy->clks)
> +		return -ENOMEM;
> +
> +	/*
> +	 * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device

Doesn't look to hacky to me, this is how you use the API if you need to
do what you propose. So I would suggest dropping "HACK: For cfg_ahb
clock, ". The remainder of the comment is good to keep (if anything it's
a TODO, to drop this sometime in the future).

> +	 * 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");

The clock is optional, but you still need to check for IS_ERR() in case
it's specified and there's an issue acquiring it.

> +
> +	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");

You can omit this print completely, because clk_bulk_prepare() and
friends will log an error line telling you which clock failed to prepare
already.

>  		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);

Here as well.

Regards,
Bjorn

>  		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 v3 2/3] phy: qcom-snps-femto-v2: add system sleep PM ops

[Bjorn Andersson]

On Thu, Jun 22, 2023 at 03:40:19PM -0400, Adrien Thierry wrote:
> The downstream driver [1] implements set_suspend(), which deals with
> both runtime and system sleep/resume. The upstream driver already has
> runtime PM ops, so add the system sleep PM ops as well, reusing the same
> code as the runtime PM ops.
> 
> [1] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c
> 
> Signed-off-by: Adrien Thierry <athierry@redhat.com>
> ---
>  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)

Won't this cause issues if you system suspend the device while it's
already runtime suspended?

Regards,
Bjorn

>  };
>  
>  static void qcom_snps_hsphy_override_param_update_val(
> -- 
> 2.40.1
> 

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

[Adrien Thierry]

Hi Bjorn,

On Thu, Jun 22, 2023 at 02:43:07PM -0700, Bjorn Andersson wrote:
> On Thu, Jun 22, 2023 at 03:40:19PM -0400, Adrien Thierry wrote:
> > The downstream driver [1] implements set_suspend(), which deals with
> > both runtime and system sleep/resume. The upstream driver already has
> > runtime PM ops, so add the system sleep PM ops as well, reusing the same
> > code as the runtime PM ops.
> > 
> > [1] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c
> > 
> > Signed-off-by: Adrien Thierry <athierry@redhat.com>
> > ---
> >  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)
> 
> Won't this cause issues if you system suspend the device while it's
> already runtime suspended?
>

I'm not sure if it would cause issues, but after reflexion and discussion
with Andrew, I think it's unnecessary to add system PM ops in the femto
PHY driver. 

In the dwc3 core, both system and runtime suspend end up calling
dwc3_suspend_common(). From there, what happens depends on the USB mode
and whether the controller is entering system or runtime suspend.

HOST mode:
  (1) system suspend on a non-wakeup controller

  The [1] if branch is taken. dwc3_core_exit() is called, which ends up
  calling phy_power_off() and phy_exit(). Those two functions decrease the
  PM runtime count at some point, so they will trigger the PHY runtime sleep
  (assuming the count is right).

  (2) runtime suspend / system suspend on a wakeup controller  

  The [1] branch is not taken. dwc3_suspend_common() calls
  phy_pm_runtime_put_sync(dwc->usb2_generic_phy). Assuming the ref count is
  right, the PHY runtime suspend op is called.

DEVICE mode:

  dwc3_core_exit() is called on both runtime and system sleep
  unless the controller is already runtime suspended.

OTG mode:
  (1) system suspend : dwc3_core_exit() is called

  (2) runtime suspend : do nothing

With that in mind:

1) Does the PHY need to implement system sleep callbacks? 

dwc3 core system sleep callback will already runtime suspend the PHY, and
also call phy_power_off() and phy_exit() for non-wakeup controllers. So,
adding system PM ops to the femto PHY driver would be redundant.

2) Should the ref_clk be disabled for runtime sleep / wakeup controller
system sleep, or only for non-wakeup controller system sleep?

I'm a little hesitant here. In my submission I'm disabling it in both, but
looking at the downstream code you provided, it seems it's only disabled
for system sleep. ref_clk is disabled by phy->set_suspend() [2] which IIUC
is only called in the system sleep path through dwc3_core_exit() [3].
Moreover, in host mode the upstream code seems to make a distinction
between 1) runtime sleep / system sleep for wakeup controller, and 2)
system sleep for non-wakeup controller where phy_power_off() and
phy_exit() are only called in the latter. This suggests the PHY is not
supposed to be in a fully powered-off state for runtime sleep and system
sleep for wakeup controller. So, it's possible the ref_clk should be kept
enabled in this case. What is your take on this? I could only disable
ref_clk in the femto phy->exit() callback to keep ref_clk enabled during
runtime sleep and make sure it's only disabled on system sleep for
non-wakeup controller.

Hoping I'm not missing anything here.

Best,

Adrien

[1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/core.c#L1988
[2] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c#L524
[3] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/dwc3/core.c#L1915

> Regards,
> Bjorn
> 
> >  };
> >  
> >  static void qcom_snps_hsphy_override_param_update_val(
> > -- 
> > 2.40.1
> > 

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

[Adrien Thierry]

Writing another email to not muddy the waters in the previous email.

I discovered that the femto PHY PM count doesn't seem to be right. Even
when the dwc3 core runtime suspends and calls
phy_pm_runtime_put_sync(dwc->usb2_generic_phy) [1], the count equals 1
after that and the PHY is not runtime suspended.

This is because on boot, the count is incremented twice because
phy_power_on() is called twice:

First:
phy_power_on+0x120/0x184
dwc3_core_init+0x68c/0xda4
dwc3_probe+0xc84/0x1304

Second:
phy_power_on+0x120/0x184
usb_phy_roothub_power_on+0x48/0xa0
usb_add_hcd+0x94/0x604
xhci_plat_probe+0x4bc/0x6e4
xhci_generic_plat_probe+0xa0/0x104

That makes the femto PHY runtime PM impossible to test at the moment. I'm
not sure if this should be fixed on the dwc3 side or the xhci side, but
this should probably be a topic for another patch series.

Best,

Adrien

[1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/core.c#L2005

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

[Bjorn Andersson]

On Tue, Jun 27, 2023 at 02:12:45PM -0400, Adrien Thierry wrote:
> Writing another email to not muddy the waters in the previous email.
> 
> I discovered that the femto PHY PM count doesn't seem to be right. Even
> when the dwc3 core runtime suspends and calls
> phy_pm_runtime_put_sync(dwc->usb2_generic_phy) [1], the count equals 1
> after that and the PHY is not runtime suspended.
> 
> This is because on boot, the count is incremented twice because
> phy_power_on() is called twice:
> 
> First:
> phy_power_on+0x120/0x184
> dwc3_core_init+0x68c/0xda4
> dwc3_probe+0xc84/0x1304
> 
> Second:
> phy_power_on+0x120/0x184
> usb_phy_roothub_power_on+0x48/0xa0
> usb_add_hcd+0x94/0x604
> xhci_plat_probe+0x4bc/0x6e4
> xhci_generic_plat_probe+0xa0/0x104
> 
> That makes the femto PHY runtime PM impossible to test at the moment. I'm
> not sure if this should be fixed on the dwc3 side or the xhci side, but
> this should probably be a topic for another patch series.
> 

Thanks for digging into this, I had forgotten about this discussion.
Unfortunately I'm confused about the (lack of?) outcome:

https://lore.kernel.org/linux-arm-msm/1648103831-12347-4-git-send-email-quic_c_sanm@quicinc.com/

Regards,
Bjorn

> Best,
> 
> Adrien
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/core.c#L2005
> 

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

[Bjorn Andersson]

On Tue, Jun 27, 2023 at 02:08:41PM -0400, Adrien Thierry wrote:
> Hi Bjorn,
> 
> On Thu, Jun 22, 2023 at 02:43:07PM -0700, Bjorn Andersson wrote:
> > On Thu, Jun 22, 2023 at 03:40:19PM -0400, Adrien Thierry wrote:
> > > The downstream driver [1] implements set_suspend(), which deals with
> > > both runtime and system sleep/resume. The upstream driver already has
> > > runtime PM ops, so add the system sleep PM ops as well, reusing the same
> > > code as the runtime PM ops.
> > > 
> > > [1] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c
> > > 
> > > Signed-off-by: Adrien Thierry <athierry@redhat.com>
> > > ---
> > >  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)
> > 
> > Won't this cause issues if you system suspend the device while it's
> > already runtime suspended?
> >
> 
> I'm not sure if it would cause issues, but after reflexion and discussion
> with Andrew, I think it's unnecessary to add system PM ops in the femto
> PHY driver. 
> 

Glad you looked further into this, I had a brief chat with Andrew and
was building a similar understanding.

> In the dwc3 core, both system and runtime suspend end up calling
> dwc3_suspend_common(). From there, what happens depends on the USB mode
> and whether the controller is entering system or runtime suspend.
> 
> HOST mode:
>   (1) system suspend on a non-wakeup controller
> 
>   The [1] if branch is taken. dwc3_core_exit() is called, which ends up
>   calling phy_power_off() and phy_exit(). Those two functions decrease the
>   PM runtime count at some point, so they will trigger the PHY runtime sleep
>   (assuming the count is right).
> 
>   (2) runtime suspend / system suspend on a wakeup controller  
> 
>   The [1] branch is not taken. dwc3_suspend_common() calls
>   phy_pm_runtime_put_sync(dwc->usb2_generic_phy). Assuming the ref count is
>   right, the PHY runtime suspend op is called.
> 
> DEVICE mode:
> 
>   dwc3_core_exit() is called on both runtime and system sleep
>   unless the controller is already runtime suspended.
> 
> OTG mode:
>   (1) system suspend : dwc3_core_exit() is called
> 
>   (2) runtime suspend : do nothing
> 
> With that in mind:
> 
> 1) Does the PHY need to implement system sleep callbacks? 
> 
> dwc3 core system sleep callback will already runtime suspend the PHY, and
> also call phy_power_off() and phy_exit() for non-wakeup controllers. So,
> adding system PM ops to the femto PHY driver would be redundant.
> 

It seems these decisions must come from the controller, in order to
handle the differences between a wakeup capable port and not. So I'm
thinking that it's correct that it should not implement this.

> 2) Should the ref_clk be disabled for runtime sleep / wakeup controller
> system sleep, or only for non-wakeup controller system sleep?
> 
> I'm a little hesitant here. In my submission I'm disabling it in both, but
> looking at the downstream code you provided, it seems it's only disabled
> for system sleep. ref_clk is disabled by phy->set_suspend() [2] which IIUC
> is only called in the system sleep path through dwc3_core_exit() [3].
> Moreover, in host mode the upstream code seems to make a distinction
> between 1) runtime sleep / system sleep for wakeup controller, and 2)
> system sleep for non-wakeup controller where phy_power_off() and
> phy_exit() are only called in the latter. This suggests the PHY is not
> supposed to be in a fully powered-off state for runtime sleep and system
> sleep for wakeup controller. So, it's possible the ref_clk should be kept
> enabled in this case. What is your take on this? I could only disable
> ref_clk in the femto phy->exit() callback to keep ref_clk enabled during
> runtime sleep and make sure it's only disabled on system sleep for
> non-wakeup controller.
> 

I suggested the same to Andrew, in our chat. Keeping sufficient
resources on, to allow the system to be woken up again by a USB device
is needed if requested. As such the handling of ref must differ between
the two cases.

It still looks a bit strange to me, having the runtime PM callbacks
prepare for wakeup from system suspend...

> Hoping I'm not missing anything here.
> 

I think you managed to capture it all. Sorry for leading you down this
incorrect path.

Regards,
Bjorn

> Best,
> 
> Adrien
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/core.c#L1988
> [2] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/phy/phy-msm-snps-hs.c#L524
> [3] https://git.codelinaro.org/clo/la/kernel/msm-5.4/-/blob/LV.AU.1.2.1.r2-05300-gen3meta.0/drivers/usb/dwc3/core.c#L1915
> 
> > Regards,
> > Bjorn
> > 
> > >  };
> > >  
> > >  static void qcom_snps_hsphy_override_param_update_val(
> > > -- 
> > > 2.40.1
> > > 
>