* [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver @ 2023-06-05 18:44 Adrien Thierry 2023-06-05 18:44 ` [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock Adrien Thierry ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Adrien Thierry @ 2023-06-05 18:44 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Konrad Dybcio, Manu Gautam, Philipp Zabel, Stephen Boyd, Vinod Koul, Wesley Cheng Cc: Adrien Thierry, linux-arm-msm, linux-phy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock 2023-06-05 18:44 [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver Adrien Thierry @ 2023-06-05 18:44 ` Adrien Thierry 2023-06-05 23:14 ` Konrad Dybcio 2023-06-21 19:48 ` Andrew Halaney 2023-06-05 18:44 ` [PATCH v2 2/2] phy: qcom-snps-femto-v2: add system sleep PM ops Adrien Thierry 2023-06-13 18:15 ` [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver Adrien Thierry 2 siblings, 2 replies; 10+ messages in thread From: Adrien Thierry @ 2023-06-05 18:44 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Vinod Koul, Kishon Vijay Abraham I, Stephen Boyd, Manu Gautam, Wesley Cheng, Philipp Zabel Cc: Adrien Thierry, linux-arm-msm, linux-phy 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock 2023-06-05 18:44 ` [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock Adrien Thierry @ 2023-06-05 23:14 ` Konrad Dybcio 2023-06-06 13:55 ` Andrew Halaney 2023-06-21 19:48 ` Andrew Halaney 1 sibling, 1 reply; 10+ messages in thread From: Konrad Dybcio @ 2023-06-05 23:14 UTC (permalink / raw) To: Adrien Thierry, Andy Gross, Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I, Stephen Boyd, Manu Gautam, Wesley Cheng, Philipp Zabel Cc: linux-arm-msm, linux-phy 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)) { ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock 2023-06-05 23:14 ` Konrad Dybcio @ 2023-06-06 13:55 ` Andrew Halaney 2023-06-06 14:35 ` Adrien Thierry 0 siblings, 1 reply; 10+ messages in thread From: Andrew Halaney @ 2023-06-06 13:55 UTC (permalink / raw) To: Konrad Dybcio Cc: Adrien Thierry, Andy Gross, Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I, Stephen Boyd, Manu Gautam, Wesley Cheng, Philipp Zabel, linux-arm-msm, linux-phy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock 2023-06-06 13:55 ` Andrew Halaney @ 2023-06-06 14:35 ` Adrien Thierry 2023-06-09 15:30 ` Konrad Dybcio 0 siblings, 1 reply; 10+ messages in thread From: Adrien Thierry @ 2023-06-06 14:35 UTC (permalink / raw) To: Andrew Halaney Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I, Stephen Boyd, Manu Gautam, Wesley Cheng, Philipp Zabel, linux-arm-msm, linux-phy 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock 2023-06-06 14:35 ` Adrien Thierry @ 2023-06-09 15:30 ` Konrad Dybcio 0 siblings, 0 replies; 10+ messages in thread From: Konrad Dybcio @ 2023-06-09 15:30 UTC (permalink / raw) To: Adrien Thierry, Andrew Halaney Cc: Andy Gross, Bjorn Andersson, Vinod Koul, Kishon Vijay Abraham I, Stephen Boyd, Manu Gautam, Wesley Cheng, Philipp Zabel, linux-arm-msm, linux-phy 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 >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock 2023-06-05 18:44 ` [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock Adrien Thierry 2023-06-05 23:14 ` Konrad Dybcio @ 2023-06-21 19:48 ` Andrew Halaney 1 sibling, 0 replies; 10+ messages in thread From: Andrew Halaney @ 2023-06-21 19:48 UTC (permalink / raw) To: Adrien Thierry Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Vinod Koul, Kishon Vijay Abraham I, Stephen Boyd, Manu Gautam, Wesley Cheng, Philipp Zabel, linux-arm-msm, linux-phy 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] phy: qcom-snps-femto-v2: add system sleep PM ops 2023-06-05 18:44 [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver Adrien Thierry 2023-06-05 18:44 ` [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock Adrien Thierry @ 2023-06-05 18:44 ` Adrien Thierry 2023-06-21 20:32 ` Andrew Halaney 2023-06-13 18:15 ` [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver Adrien Thierry 2 siblings, 1 reply; 10+ messages in thread From: Adrien Thierry @ 2023-06-05 18:44 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Vinod Koul, Kishon Vijay Abraham I Cc: Adrien Thierry, linux-arm-msm, linux-phy 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] phy: qcom-snps-femto-v2: add system sleep PM ops 2023-06-05 18:44 ` [PATCH v2 2/2] phy: qcom-snps-femto-v2: add system sleep PM ops Adrien Thierry @ 2023-06-21 20:32 ` Andrew Halaney 0 siblings, 0 replies; 10+ messages in thread From: Andrew Halaney @ 2023-06-21 20:32 UTC (permalink / raw) To: Adrien Thierry Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Vinod Koul, Kishon Vijay Abraham I, linux-arm-msm, linux-phy 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver 2023-06-05 18:44 [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver Adrien Thierry 2023-06-05 18:44 ` [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock Adrien Thierry 2023-06-05 18:44 ` [PATCH v2 2/2] phy: qcom-snps-femto-v2: add system sleep PM ops Adrien Thierry @ 2023-06-13 18:15 ` Adrien Thierry 2 siblings, 0 replies; 10+ messages in thread From: Adrien Thierry @ 2023-06-13 18:15 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Konrad Dybcio, Manu Gautam, Philipp Zabel, Stephen Boyd, Vinod Koul, Wesley Cheng Cc: linux-arm-msm, linux-phy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-06-21 20:33 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-05 18:44 [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver Adrien Thierry 2023-06-05 18:44 ` [PATCH v2 1/2] phy: qcom-snps-femto-v2: properly enable ref clock Adrien Thierry 2023-06-05 23:14 ` Konrad Dybcio 2023-06-06 13:55 ` Andrew Halaney 2023-06-06 14:35 ` Adrien Thierry 2023-06-09 15:30 ` Konrad Dybcio 2023-06-21 19:48 ` Andrew Halaney 2023-06-05 18:44 ` [PATCH v2 2/2] phy: qcom-snps-femto-v2: add system sleep PM ops Adrien Thierry 2023-06-21 20:32 ` Andrew Halaney 2023-06-13 18:15 ` [PATCH v2 0/2] Clock fixes for qcom-snps-femto-v2 PHY driver Adrien Thierry
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).