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

[Adrien Thierry <athierry@redhat.com>]

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@redhat.com>
> > > > ---
> > > >  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
> > 


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy