Hi Bjorn, thanks for your reply! On Mon, May 29, 2023 at 02:16:29PM -0700, Bjorn Andersson wrote: > On Mon, May 29, 2023 at 02:56:36PM -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. > > > > Good catch! > > A side note though, clk_disable_unused() has no way to take kernel > modules into consideration today, and it doesn't handle the case where > clock drivers are built as modules appropriately. > Work has started to address this, but as of todaybooting the system > without clk_ignore_unused is not recommended... > For my understanding, do you have an example of a situation that would fail with modules when not using clk_ignore_unused? > > To fix this, add calls to clk_prepare_enable/clk_disable_unprepare at > > the proper places. > > > > If I parse the downstream kernel correctly the refclock should be > turned off across runtime and system suspend as well. > Which downstream kernel are you using? I'm not seing a system suspend callback in mine [1]. refclock should be turned off on runtime suspend in my patch, in qcom_snps_hsphy_suspend, which is called by qcom_snps_hsphy_runtime_suspend. [1] https://git.codelinaro.org/clo/la/kernel/ark-5.14/-/blob/kernel.lnx.5.14.r2-rel/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > Regards, > Bjorn > > > Signed-off-by: Adrien Thierry <athierry@xxxxxxxxxx> > > --- > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 20 +++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 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..8abf482e81a8 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > @@ -166,6 +166,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > > } > > > > clk_disable_unprepare(hsphy->cfg_ahb_clk); > > + clk_disable_unprepare(hsphy->ref_clk); > > return 0; > > } > > > > @@ -181,6 +182,12 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > > return ret; > > } > > > > + ret = clk_prepare_enable(hsphy->ref_clk); > > + if (ret) { > > + dev_err(&hsphy->phy->dev, "failed to enable ref clock\n"); > > + return ret; > > + } > > + > > return 0; > > } > > > > @@ -380,10 +387,16 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > goto poweroff_phy; > > } > > > > + ret = clk_prepare_enable(hsphy->ref_clk); > > + if (ret) { > > + dev_err(&phy->dev, "failed to enable ref clock, %d\n", ret); > > + goto disable_ahb_clk; > > + } > > + > > 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_ref_clk; > > } > > > > usleep_range(100, 150); > > @@ -391,7 +404,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_ref_clk; > > } > > > > qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_CFG0, > > @@ -448,6 +461,8 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > > > return 0; > > > > +disable_ref_clk: > > + clk_disable_unprepare(hsphy->ref_clk); > > disable_ahb_clk: > > clk_disable_unprepare(hsphy->cfg_ahb_clk); > > poweroff_phy: > > @@ -462,6 +477,7 @@ static int qcom_snps_hsphy_exit(struct phy *phy) > > > > reset_control_assert(hsphy->phy_reset); > > clk_disable_unprepare(hsphy->cfg_ahb_clk); > > + clk_disable_unprepare(hsphy->ref_clk); > > regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); > > hsphy->phy_initialized = false; > > > > -- > > 2.40.1 > > Best, Adrien
