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

[Adrien Thierry]

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 at 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