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

[Adrien Thierry <athierry@redhat.com>]

Thanks for your feedback Konrad and Andrew!

On Tue, Jun 06, 2023 at 08:55:16AM -0500, Andrew Halaney wrote:
> 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@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.
>

Agree with Andrew. Since cfg_ahb is always NULL, I'm certainly "wasting"
an array cell here but I think it also better highlights the fact that
it's a hack and that cfg_ahb needs to be properly wired in the DTs. As for
using the bulk optional API, I'm fine with both!

> > > +
> > > +	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.
>

devm_clk_get_optional() will return NULL for cfg_ahb, but AFAIU NULL
serves as a dummy clock [1] with which the clock API deals gracefully. The
various functions like clk_prepare(), clk_enable() check if the clock is
NULL and return 0 immediately if that's the case (see for instance [2]).

[1] https://elixir.bootlin.com/linux/v6.4-rc5/source/include/linux/clk.h#L514
[2] https://elixir.bootlin.com/linux/v6.4-rc5/source/drivers/clk/clk.c#L1045

Best,
Adrien

> Thanks,
> Andrew
>