On Tue, May 30, 2023 at 07:34:41PM -0700, Bjorn Andersson wrote: > On Tue, May 30, 2023 at 02:46:05PM -0400, Adrien Thierry wrote: > > 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? > > > > The prime example relates to the display clocks, where the bootloader > typically leave clocks on and at lateinit we haven't yet loaded enough > modules to bring up the display. And to make matters worse, the code > ends up disabling the PLL feeding the clock tree without first disabling > some of the muxes - which has side effects... > > Another case, although much less concerning in the short run, is when > you have any of the clock drivers built as modules. clk_disable_unused() > will be invoked before they are loaded, so your expectation that unused > clocks are turned off is just not fulfilled. > Thanks for the explanation! > > > > 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. > > > > Forgive me, but isn't [1] the driver you're modifying? > > I'm looking at [2], with set_suspend() being invoked from the runtime > and system suspend/resume handlers. > > > [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 > > [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#L908 > Thank you. The femto PHY driver is not using set_suspend(), but has runtime PM ops. Is it ok if I add the system sleep PM ops as well with SET_SYSTEM_SLEEP_PM_OPS() ? > Regards, > Bjorn > > > > > > 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 > >
