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

[Andrew Halaney]

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